Page MenuHomePhabricator

[avalanche] Poll for transactions added to the mempool
ClosedPublic

Authored by Fabien on Jan 24 2024, 20:43.

Details

Reviewers
PiRK
Group Reviewers
Restricted Project
Commits
rABCae5ecec37497: [avalanche] Poll for transactions added to the mempool
Summary

This is limited to tx reaching the mempool for now, which is incomplete, and limited to the poll gated behind a flag off by default.

This is done via the validation interface so get rid of the incredilbly complicated locking scheme that makes it impossible to not cause deadlocks if called from validation.cpp or txmempool.cpp. This is fine though because it's only starting the vote on the item and doesn't need to access the mempool, nor does it need to ensure consistency with cs_main + mempool.cs.

Depends on D15270.

Test Plan
./test/functional/test_runner.py abc_p2p_avalanche_transaction_voting

Diff Detail

Event Timeline

Fabien requested review of this revision.Jan 24 2024, 20:43
bytesofman added inline comments.
src/avalanche/processor.cpp
140 ↗(On Diff #44554)

is this supposed to be m_processor and not m_preConsensus?

src/avalanche/processor.h
281 ↗(On Diff #44554)

what is this? how is it different/related to

argsman.GetBoolArg("-avalanchepreconsensus",
                           DEFAULT_AVALANCHE_PRECONSENSUS)));
test/functional/abc_p2p_avalanche_transaction_voting.py
156 ↗(On Diff #44554)

is this testing the feature? I don't see the gate being set to true.

Do we need a test for "by default, this doesn't happen" and "gate default overwritten, now this thing does happen"

PiRK added a subscriber: PiRK.
PiRK added inline comments.
test/functional/abc_p2p_avalanche_transaction_voting.py
156 ↗(On Diff #44554)

It is currently true for the entire test. See self.extra_args in set_test_params

Ideally we should test "by default, this doesn't happen", but I'm not sure how easy it is to test for the absence of a behavior. How long do we observe until we conclude that it is definitely not going to happen?

This revision is now accepted and ready to land.Jan 25 2024, 07:47
src/avalanche/processor.cpp
140 ↗(On Diff #44554)

This is an event handler, forwarding the event to the processor instance that will handle it.

src/avalanche/processor.h
281 ↗(On Diff #44554)

This makes the flag a member of the Processor class. It is set once at construction time instead of looking at the command line/config file args everytime it's needed, avoiding a map lookup. Since we will check this for every transaction we don't want to spend our time doing lookups and this is what this cached flag achieves.

test/functional/abc_p2p_avalanche_transaction_voting.py
156 ↗(On Diff #44554)

Testing for absence of a feature is by essence not a good test :).

Also note that the reconcile function is already tested with unit tests so we only want to check the integration here.

src/avalanche/processor.h
281 ↗(On Diff #44554)

And since it's only set once at construction time it should be const, let me fix this