[avalanche] Poll for transactions added to the mempool
ClosedPublic
Actions

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

Repository

rABC Bitcoin ABC

Branch

add_tx_to_reconcile

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 26627
Build 52823: Build Diff	build-without-wallet · lint-circular-dependencies · build-debug · build-clang · build-diff · build-clang-tidy
Build 52822: arc lint + arc unit

Event Timeline

Fabien created this revision.Jan 24 2024, 20:43

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 24 2024, 20:43

Fabien requested review of this revision.Jan 24 2024, 20:43

Harbormaster completed remote builds in B26627: Diff 44554.Jan 24 2024, 21:00

bytesofman added a subscriber: bytesofman.Jan 24 2024, 21:19

bytesofman added inline comments.

src/avalanche/processor.cpp
140	is this supposed to be `m_processor` and not `m_preConsensus`?
src/avalanche/processor.h
281	what is this? how is it different/related to argsman.GetBoolArg("-avalanchepreconsensus", DEFAULT_AVALANCHE_PRECONSENSUS)));
test/functional/abc_p2p_avalanche_transaction_voting.py
156	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 accepted this revision.Jan 25 2024, 07:47

PiRK added a subscriber: PiRK.

PiRK added inline comments.

test/functional/abc_p2p_avalanche_transaction_voting.py
156	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

Fabien added inline comments.Jan 25 2024, 08:52

src/avalanche/processor.cpp
140	This is an event handler, forwarding the event to the processor instance that will handle it.
src/avalanche/processor.h
281	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	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.