[avalanche] Limit the finalized txs radix tree size so it always fits in the next block
Needs RevisionPublic
Actions

Authored by Fabien on Wed, Mar 19, 20:48.

Details

Reviewers

PiRK
roqqit

Group Reviewers

Restricted Project

Summary

This delays adding the transactions that won't fit the next block. This is a step toward making use of the radix tree for the block template.
For now the txs that are delayed remain sitting there, this will be fixed in the next diff.

Depends on D17829.

Test Plan

ninja all check-all

Diff Detail

Repository

rABC Bitcoin ABC

Branch

avalanche_tx_block_full

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 32787
Build 65062: Build Diff	build-diff · build-clang-tidy · build-debug · build-clang · lint-circular-dependencies · build-without-wallet
Build 65061: arc lint + arc unit

Event Timeline

Fabien created this revision.Wed, Mar 19, 20:48

Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Mar 19, 20:48

Fabien requested review of this revision.Wed, Mar 19, 20:48

Fabien added a child revision: D17832: [avalanche] Poll for the transactions that didn't make it into the last block.Wed, Mar 19, 20:50

Harbormaster completed remote builds in B32774: Diff 53187.Wed, Mar 19, 21:02

PiRK added a subscriber: PiRK.Thu, Mar 20, 08:52

PiRK added inline comments.

test/functional/test_framework/wallet.py
425 ↗	(On Diff #53187)	this seems unrelated, the diff does not call this method

PiRK added inline comments.Thu, Mar 20, 08:56

src/txmempool.cpp
556 ↗	(On Diff #53187)	I find "Dropping" a bit confusing and vague. What about "Not finalizing tx %s as ...." ?

PiRK added inline comments.Thu, Mar 20, 08:57

src/txmempool.cpp
556 ↗	(On Diff #53187)	"Not keeping track of finalized tx %s...." ?

Fabien marked an inline comment as done.Thu, Mar 20, 09:38

Fabien added inline comments.

src/txmempool.cpp
556 ↗	(On Diff #53187)	What about "Delay storing finalized tx..." ?
test/functional/test_framework/wallet.py
425 ↗	(On Diff #53187)	It does indirectly via send_self_transfer_chain(). It's so that I can pass down the target size when building a chain of txs
447 ↗	(On Diff #53187)	here

PiRK accepted this revision.Thu, Mar 20, 11:08

PiRK added inline comments.

src/txmempool.cpp
556 ↗	(On Diff #53187)	Sounds better. Not sure if there is a way to say that we are still keeping the tx, just not the finalized status. But it is not a log message most people will display, so not a big deal.

This revision is now accepted and ready to land.Thu, Mar 20, 11:08

Reword the log

Harbormaster completed remote builds in B32787: Diff 53203.Thu, Mar 20, 13:20

roqqit requested changes to this revision.Thu, Mar 20, 17:25

roqqit added a subscriber: roqqit.

roqqit added inline comments.

src/txmempool.cpp
545–551	can improve taste (and make it more robust to future changes)
562	this comment feels out of place. maybe bump it to after `finalizedTxIds.clear();`
565
test/functional/abc_p2p_avalanche_transaction_finalization.py
19–37	move the comment to where the value is
248–250	is this flaky? finalize_tx() answers ACCEPTED to all polls while only looking for one txid at a time, so I think it is possible to finalized another txid before this specific one

This revision now requires changes to proceed.Thu, Mar 20, 17:25

Fabien added inline comments.Thu, Mar 20, 19:55

test/functional/abc_p2p_avalanche_transaction_finalization.py
248–250	it's a single chain of txs. So this test doesn't care about the order the items are voted on, ultimately they all get accepted but only the first 4 in the chain can fit the radix tree.