Page MenuHomePhabricator

[avalanche] Refactor the vote records to inv extraction loop
ClosedPublic

Authored by Fabien on Oct 13 2021, 14:55.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Maniphest Tasks
Restricted Maniphest Task
Commits
rABC9fb3623e16e1: [avalanche] Refactor the vote records to inv extraction loop
Summary

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.

Test Plan
ninja all check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
avalanche_factorize_inv_loop
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 17062
Build 33957: Build Difflint-circular-dependencies · build-without-wallet · build-diff · build-debug · build-clang · build-clang-tidy
Build 33956: arc lint + arc unit

Event Timeline

Fabien requested review of this revision.Oct 13 2021, 14:55
PiRK added inline comments.
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.

deadalnix requested changes to this revision.Oct 14 2021, 14:48
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
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.

This revision now requires changes to proceed.Oct 14 2021, 14:48

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.

deadalnix requested changes to this revision.Oct 15 2021, 18:45
deadalnix added inline comments.
src/avalanche/processor.cpp
540 ↗(On Diff #30576)

I see no good reason to use iterators here rather than a range style loop.

559 ↗(On Diff #30576)

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 ↗(On Diff #30576)

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.

This revision now requires changes to proceed.Oct 15 2021, 18:45

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 ?

Use a const ref for the range

deadalnix added inline comments.
src/avalanche/processor.cpp
550

Maybe this could return a bool, so that you know if you need to return or continue.

This revision is now accepted and ready to land.Oct 18 2021, 18:12

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.

Fabien edited the summary of this revision. (Show Details)
Fabien planned changes to this revision.Oct 20 2021, 17:03

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.

This revision is now accepted and ready to land.Oct 21 2021, 08:13