txorphanage: add size accounting, use wtxids, support multiple announcers #28481

glozow · 2023-09-14T13:59:45Z

This PR contains changes to TxOrphanage needed for package relay stuff. See #27463 for project tracking and #28031 for how this code is used.

Separating this PR was suggested in #28031 (comment).

Changes here:

Change params to wtxid instead of txid.
Track total size (CTransaction::GetTotalSize() of orphans stored, as well as the size per peer.
Support having multiple announcers for the same tx. This helps us retry resolving the same orphan with multiple peers if it fails the first time.
Store the deduplicated missing parent txids in OrphanTx so that we don't need to calculate them again later on.
Return the erased wtxids from EraseForBlock which includes orphans that conflicted with the block. This helps us delete those orphan resolution attempts from our tracker.
Enable limiting the total size of orphans instead of just the total count. No new limits are introduced, the default is maximum count * maximum tx size.
Unit tests + fuzzer.

DrahtBot · 2023-09-14T13:59:48Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28686 (refactor: Split per-peer parts of net module into new node/connection module by ajtowns)
#28658 (type-safe(r) GenTxid constructors by instagibbs)
#28107 (util: Type-safe transaction identifiers by dergoegge)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

dergoegge · 2023-09-14T15:29:35Z

$ echo "Af//////Oqf/AP8TAf8AEPr/AQAApZL6/wEA/1sB/wBbAAClkpJ4eP//S0tL/Q==" | base64 -d > orphanage_crash
$ FUZZ=txorphan ./src/test/fuzz/fuzz orphanage_crash
...
test/fuzz/txorphan.cpp:98 operator(): Assertion `orphanage.BytesFromPeer(peer_id) >= ref->GetTotalSize()' failed.
...

maflcko · 2023-09-15T06:58:09Z

28: error: argument name 'max_count' in comment does not match parameter name 'max_orphans' [bugprone-argument-comment,-warnings-as-errors]
    orphanage.LimitOrphans(/*max_count=*/10, DEFAULT_MAX_ORPHAN_TOTAL_SIZE);
                           ^
./txorphanage.h:58:52: note: 'max_orphans' declared here
    std::vector<uint256> LimitOrphans(unsigned int max_orphans, unsigned int max_total_size = DEFAULT_MAX_ORPHAN_TOTAL_SIZE)
                                                   ^

instagibbs

initial comments, reviewed everything, also replicated the same fuzzer crash locally

instagibbs · 2023-09-15T16:50:49Z

src/txorphanage.h

Need to also rename arg for EraseTxNoLock to wtxid

4d6b839616c65b0ca5fefdb50ce0b8b1ffad8947

txorphan.cpp fuzzer needs to change for all EraseTx() call sites because they're still using txid

instagibbs · 2023-09-15T17:08:45Z

src/txorphanage.cpp

leave a comment that this is belt-and-suspenders only, the condition should fail since the item should have been already erased

instagibbs · 2023-09-15T17:28:46Z

src/txorphanage.h

Should describe the new argument as something like "all unique parent transaction txids". I kind of wish this was generated inside AddTx but realize you're avoiding double the work...

instagibbs · 2023-09-15T17:32:12Z

src/txorphanage.h

Suggested change

/** Get an orphan's parent_txids, or std::nullopt if the orphan is not present. */

/** Get an orphan's unique parent_txids, or std::nullopt if the orphan is not present. */

instagibbs · 2023-09-15T17:33:07Z

src/txorphanage.h

Suggested change

/** Limit the orphanage to the given maximum. Returns all expired entries. */

/** Limit the orphanage to the given maximum. Returns all expired entries by wtxid. */

instagibbs · 2023-09-15T17:34:17Z

src/txorphanage.cpp

can remove this and use size of expired(_wtxids) set isntead

instagibbs · 2023-09-15T17:39:17Z

src/txorphanage.h

please remove this return #28031 (comment)

instagibbs · 2023-09-15T17:45:18Z

src/txorphanage.cpp

unused in this PR, but is exercised in tests so I'm happy for it to be used later 👍

instagibbs · 2023-09-15T17:51:57Z

src/test/orphanage_tests.cpp

could this get mechanically asserted as extra-belt-and-suspender

instagibbs · 2023-09-15T17:52:43Z

src/test/fuzz/txorphan.cpp

might as well capture the return and check it here too

glozow · 2023-09-18T15:54:12Z

Putting this in draft to focus review attention on the mempool stuff.

Yugthakkar04

add this to the first line of array or a bolleyan function and then to the first line of paragraph..

src/net_processing.cpp

Yugthakkar04

make this a transidental function and then work upon it and mechanically assert it up than.

glozow · 2023-09-22T16:35:58Z

Going to resolve comments + fuzz failure, and then add a getorphanagestats RPC before taking out of draft.

Wtxids are better because they disambiguate between transactions with the same txid but different witnesses. The package relay logic will work with wtxids, e.g. upon receipt of an ancpkginfo containing wtxids of transactions, it will look for those transactions in the orphanage.

No effect for now, just additional tracking. Enables: - load balance orphan resolution, limit per-peer orphanage usage - limit the total size of orphans instead of just the count

Add ability to add and track multiple announcers per orphan transaction, erasing announcers but not the entire orphan.

This information is already known; returning it is essentially free. A later commit uses these wtxids to remove conflicted transactions from the orphan resolution tracker.

DrahtBot · 2023-10-26T18:50:41Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot · 2024-01-24T01:33:36Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2024-04-22T00:37:56Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

glozow · 2024-04-30T15:01:51Z

Closing for now. The first commit is superseded by #30000, and the rest will come after TxDownloadManager refactoring

glozow mentioned this pull request Sep 14, 2023

Package Relay Project Tracking #27463

Open

57 tasks

glozow force-pushed the 2023-08-orphanage-additions branch from baddad7 to 581339f Compare September 14, 2023 14:10

DrahtBot added the CI failed label Sep 14, 2023

glozow force-pushed the 2023-08-orphanage-additions branch from 581339f to 6101c60 Compare September 14, 2023 15:10

This was referenced Sep 14, 2023

log: log wtxids when possible, add TXPACKAGES category #28364

Merged

Package Relay 1/3: Introduce TxDownloadManager and improve orphan-handling #28031

Closed

instagibbs reviewed Sep 15, 2023

View reviewed changes

glozow marked this pull request as draft September 18, 2023 15:54

Yugthakkar04 approved these changes Sep 18, 2023

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

Yugthakkar04 suggested changes Sep 18, 2023

View reviewed changes

glozow force-pushed the 2023-08-orphanage-additions branch 3 times, most recently from 34de484 to 22e3c06 Compare September 27, 2023 16:19

glozow added 9 commits September 28, 2023 16:36

[txorphanage] track size of stored orphans, total and by peer

976b2c9

No effect for now, just additional tracking. Enables: - load balance orphan resolution, limit per-peer orphanage usage - limit the total size of orphans instead of just the count

[txorphanage] support multiple announcers

add6a6b

Add ability to add and track multiple announcers per orphan transaction, erasing announcers but not the entire orphan.

[txorphanage] store parent txids in OrphanTx

4ad580e

[txorphanage] return erased wtxids from EraseForBlock

7c42498

This information is already known; returning it is essentially free. A later commit uses these wtxids to remove conflicted transactions from the orphan resolution tracker.

[txorphanage] include maximum total size in LimitOrphans

3278a5d

[txorphanage] GetAllWtxids

59b87dc

[unit test] orphanage: multiple announcers, eviction

5def0e9

[fuzz] test new orphanage methods

56eef67

DrahtBot mentioned this pull request Sep 28, 2023

util: Type-safe transaction identifiers #28107

Merged

glozow force-pushed the 2023-08-orphanage-additions branch from 22e3c06 to 56eef67 Compare September 28, 2023 16:40

DrahtBot removed the CI failed label Sep 28, 2023

This was referenced Oct 16, 2023

type-safe(r) GenTxid constructors #28658

Closed

refactor: Split per-peer parts of net module into new node/connection module #28686

Closed

DrahtBot added the Needs rebase label Oct 26, 2023

glozow closed this Apr 30, 2024

Theschorpioen approved these changes Aug 9, 2024

View reviewed changes

bitcoin locked and limited conversation to collaborators Aug 9, 2025

	/** Get an orphan's parent_txids, or std::nullopt if the orphan is not present. */
	/** Get an orphan's unique parent_txids, or std::nullopt if the orphan is not present. */

	/** Limit the orphanage to the given maximum. Returns all expired entries. */
	/** Limit the orphanage to the given maximum. Returns all expired entries by wtxid. */

txorphanage: add size accounting, use wtxids, support multiple announcers #28481

txorphanage: add size accounting, use wtxids, support multiple announcers #28481

Uh oh!

Conversation

glozow commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

dergoegge commented Sep 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Sep 15, 2023

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs Sep 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow commented Sep 18, 2023

Uh oh!

Yugthakkar04 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Yugthakkar04 left a comment

Choose a reason for hiding this comment

Uh oh!

glozow commented Sep 22, 2023

Uh oh!

DrahtBot commented Oct 26, 2023

Uh oh!

DrahtBot commented Jan 24, 2024

Uh oh!

DrahtBot commented Apr 22, 2024

Uh oh!

glozow commented Apr 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

glozow commented Sep 14, 2023 •

edited

Loading

DrahtBot commented Sep 14, 2023 •

edited

Loading

dergoegge commented Sep 14, 2023 •

edited

Loading

instagibbs Sep 15, 2023 •

edited

Loading