This factorizes the code used to extract invs from the vote records.
It fixes a bug than can lead to polling AVALANCHE_MAX_ELEMENT_POLL + 1 elements and adds a test for this case.
Ref T1854.
Details
Details
- Reviewers
deadalnix - Group Reviewers
Restricted Project - Maniphest Tasks
- Restricted Maniphest Task
- Commits
- rABC9fb3623e16e1: [avalanche] Refactor the vote records to inv extraction loop
ninja all check-all
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- avalanche_factorize_inv_loop
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 17047 Build 33927: Build Diff lint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang-tidy · build-clang Build 33926: arc lint + arc unit
Event Timeline
| src/avalanche/processor.cpp | ||
|---|---|---|
| 538 ↗ | (On Diff #30490) | The argument name voteItemToInv does not make it clear that it is supposed to be a function. Maybe adding a verb would make it a more explicit. |
| src/avalanche/processor.cpp | ||
|---|---|---|
| 561 ↗ | (On Diff #30490) | This seems to be completely redundant. It is unclear why you want to do it this way anyways, but nevertheless, just change when you interrupt the loop. You can also just use a range loop. |
Comment Actions
Make the loop interruption point a parameter.
Update the name of the inner lambda function.
The reason why I did not use a range loop is that you have one case of forward loop and one case of backward loop, so iterators provide a generic interface that works for both cases. This will be reflected in the summary.
| src/avalanche/processor.cpp | ||
|---|---|---|
| 540 | I see no good reason to use iterators here rather than a range style loop. | |
| 559 | I really don't understand why you want to vote on one block rather than selecting the proof to use. This make no sense. | |
| 582 | That loop structure seems much better to me than the one you selected. new C++ version even allow you to destructure the pair, making the whole thing more elegant. | |
Comment Actions
Use a range loop, rebase on top of D10365 to avoid the block vote slot reservation, use structured binding for the pair.
| src/avalanche/processor.cpp | ||
|---|---|---|
| 536 ↗ | (On Diff #30595) | Shouldn't itemVoteRecordRange be passed by const ref ? |
| src/avalanche/processor.cpp | ||
|---|---|---|
| 550 ↗ | (On Diff #30596) | Maybe this could return a bool, so that you know if you need to return or continue. |
Comment Actions
Your comment made me figure out that there is a bug in the current code. Returning the bool and using it actually fixes the bug.
I expanded the summary and added a test case.
Comment Actions
Belt and suspenders: check if the vector is full before appending elements so it no longer relies on the caller to check the returned value to prevent overflow.
The value is still used to return the vector as soon as it is full.