[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 26645
Build 52858: Build Diff	lint-circular-dependencies · build-without-wallet · build-clang · build-debug · build-clang-tidy · build-diff
Build 52857: 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 ↗	(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 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 ↗	(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

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

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.