fix: accept up to 64 content items, not just 8

[carver]

What was wrong?

Fix #1484

How was it fixed?

Increase the size of the Accept Bitlist

To-Do

• Clean up commit history and use conventional commits.

[KolbyML]

[carver]

Hm, well it's concerning that this changes the actual message encoding, which is to be expected I suppose:

--- types::portal_wire::test::message_encoding_accept stdout ----
thread 'types::portal_wire::test::message_encoding_accept' panicked at ethportal-api/src/types/portal_wire.rs:783:9:
assertion `left == right` failed
  left: "0x07010206000000010000000000000001"
 right: "0x070102060000000101"

So why is the code on master working, if other clients are expecting 64 bits of accepted content IDs and we're only sending 8?

[carver]

Besides Kolby, looks like @ogenev was the last to touch these lines of code (in early 2023 😆 ). Any idea what's going on here?

If either of you immediately see it, great. If not, I'll hunt it down next time I'm Flamingo.

[KolbyML]

Besides Kolby, looks like @ogenev was the last to touch these lines of code (in early 2023 😆 ). Any idea what's going on here?

If either of you immediately see it, great. If not, I'll hunt it down next time I'm Flamingo.

This PR updates let mut content_keys = BitList::with_capacity(8).unwrap(); to let mut content_keys = BitList::with_capacity(64).unwrap();. I think this should remain let mut content_keys = BitList::with_capacity(8).unwrap();. As shouldn't the capacity of the bitlist be the amount of keys sent in the offer?. The test in the spec simulates 8 pieces of content were offered

This test case has a bitlist of capacity 8. So I think the overall PR is good, we were just not supposed to change the test's bitlist capacity

This PR changes the BitList so the max BitList capacity can be 64, but the capacity chosen is determined by the offer sent

[carver]

Hm, yeah. So obviously we do need to test that the message is capable of holding 64 items, it's just that this test assumes only 8 max in the encoded hex. And the bitlist is variable size, depending on the message.

[KolbyML]

Hm, yeah. So obviously we do need to test that the message is capable of holding 64 items, it's just that this test assumes only 8 max in the encoded hex. And the bitlist is variable size, depending on the message.

If you want to test that we could updated the expected_encoded to let expected_encoded = "0x070102060000000101"; or add another test which tests for using a 64 bit capacity.

This would make a good hive test, and I can make that.

I don't think this is a blocker to this PR, though. Although I agree we should add an end to end test for this which I can do quick

[carver]

add another test which tests for using a 64 bit capacity.

Yup, this was the direction I was headed.

I agree we should add an end to end test for this which I can do quick

That sounds great

I don't think this is a blocker to this PR, though.

I think it is. We are kind of sloppy about this as a team sometimes, because the incremental cost of adding a test for each scenario can be pretty high. But as much as possible we should include a test with every PR. This one should be straightforward.

[carver]

I don't think this is a blocker to this PR, though. Although I agree we should add an end to end test for this which I can do quick

Oh, I'm realizing I probably misinterpreted you. You meant that the hive test is not a blocker for this? Yeah I agree with that. Just the local test, which is now added.

[KolbyML]

I don't think this is a blocker to this PR, though.

I think it is. We are kind of sloppy about this as a team sometimes, because the incremental cost of adding a test for each scenario can be pretty high. But as much as possible we should include a test with every PR. This one should be straightforward.

I think this is a misunderstanding.

What I am saying is we can fix the test broken by this PR, and add a test if wanted as well, well fixing the test broken by this PR.

Hm, yeah. So obviously we do need to test that the message is capable of holding 64 items, it's just that this test assumes only 8 max in the encoded hex. And the bitlist is variable size, depending on the message.

I said that in response to this. It takes less time than it took to write this message to add a test, making sure the accept message can now send a bitlist with 64 items. I was kind of confused by this response because I thought a commit could just be pushed, which

• kept the test from the spec
• but added a test to make sure the 64-length bitlist works.

None of this takes time, so I don't see why any of it would block this PR. Currently, CI is failing, so I assumed we could fix the tests. As I said before, my understanding was that tests would be added. I also mentioned I would be writing a Hive test, which is good as it would help all clients. What I meant by I don't think this is a blocker to this PR wasn't that I don't think we should add a test for testing 64-length bitlists in this PR, but that nothing is wrong with the PR that needs to be discussed and I assumed the tests would be added.

I would say there is a blocker if something drastically wrong with the PR needs to be discussed or if there is a misunderstanding. I think everybody is on the same page with this PR, so I don't think there is a blocker that needs to be discussed.

Sorry for the confusion. I will try to better articulate what I mean next time if I ever refer to something as not being a blocker again