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

@@ -33,8 +36,8 @@ class TxOrphanage {
 */
 CTransactionRef GetTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);

- /** Erase an orphan by txid */
- int EraseTx(const uint256& txid) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);


Need to also rename arg for EraseTxNoLock to wtxid

4d6b839

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

@@ -118,6 +139,8 @@ void TxOrphanage::EraseForPeer(NodeId peer)
 }
 }
 if (nErased > 0) LogPrint(BCLog::TXPACKAGES, "Erased %d orphan tx from peer=%d\n", nErased, peer);
+ if (!Assume(m_peer_bytes_used.count(peer) == 0)) m_peer_bytes_used.erase(peer);


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

@@ -22,7 +22,7 @@ class TxOrphanage {
 public:
 /** Add a new orphan transaction. If the tx already exists, add this peer to its list of announcers.
 @returns true if the transaction was added as a new orphan. */
- bool AddTx(const CTransactionRef& tx, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
+ bool AddTx(const CTransactionRef& tx, NodeId peer, const std::vector<uint256>& parent_txids) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);


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

@@ -92,6 +92,9 @@ class TxOrphanage {
 * remove this peer's entry from the map. */
 void SubtractOrphanBytes(unsigned int size, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(m_mutex);

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


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


 /** Erase all orphans included in or invalidated by a new block */
 void EraseForBlock(const CBlock& block) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);

- /** Limit the orphanage to the given maximum */
- void LimitOrphans(unsigned int max_orphans) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
+ /** Limit the orphanage to the given maximum. Returns all expired entries. */


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

 {
 LOCK(m_mutex);

+ std::vector<uint256> expired;
 unsigned int nEvicted = 0;


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

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

src/txorphanage.h

- void EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
+ /** Maybe erase all orphans announced by a peer (eg, after that peer disconnects). If an orphan
+ * has been announced by another peer, don't erase, just remove this peer from the list of announcers. */
+ std::vector<uint256> EraseForPeer(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);


please remove this return #28031 (comment)

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

src/txorphanage.cpp

@@ -278,7 +278,7 @@ bool TxOrphanage::HaveTxToReconsider(NodeId peer)
 return false;
 }

-void TxOrphanage::EraseForBlock(const CBlock& block)
+std::vector<uint256> TxOrphanage::EraseForBlock(const CBlock& block)


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

+ tx.vin.emplace_back(CTxIn(outpoint));
+ }
+ }
+ // Ensure txid != wtxid


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

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

src/test/fuzz/txorphan.cpp

+ // check EraseForBlock
+ CBlock block;
+ block.vtx.emplace_back(tx);
+ orphanage.EraseForBlock(block);


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

56 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

Draft

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 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

glozow commented Sep 14, 2023 •

edited

DrahtBot commented Sep 14, 2023 •

edited

dergoegge commented Sep 14, 2023 •

edited

maflcko commented Sep 15, 2023

instagibbs left a comment

instagibbs Sep 15, 2023

instagibbs Sep 15, 2023

instagibbs Sep 15, 2023

instagibbs Sep 15, 2023 •

edited

instagibbs Sep 15, 2023

instagibbs Sep 15, 2023

instagibbs Sep 15, 2023

instagibbs Sep 15, 2023

instagibbs Sep 15, 2023

instagibbs Sep 15, 2023

instagibbs Sep 15, 2023

glozow commented Sep 18, 2023

Yugthakkar04 left a comment

Yugthakkar04 left a comment

glozow commented Sep 22, 2023

DrahtBot commented Oct 26, 2023

DrahtBot commented Jan 24, 2024

DrahtBot commented Apr 22, 2024

glozow commented Apr 30, 2024

	/** 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

Conversation

glozow commented Sep 14, 2023 • edited

DrahtBot commented Sep 14, 2023 • edited

Reviews

Conflicts

dergoegge commented Sep 14, 2023 • edited

maflcko commented Sep 15, 2023

instagibbs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs Sep 15, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glozow commented Sep 18, 2023

Yugthakkar04 left a comment

Choose a reason for hiding this comment

Yugthakkar04 left a comment

Choose a reason for hiding this comment

glozow commented Sep 22, 2023

DrahtBot commented Oct 26, 2023

DrahtBot commented Jan 24, 2024

DrahtBot commented Apr 22, 2024

glozow commented Apr 30, 2024

glozow commented Sep 14, 2023 •

edited

DrahtBot commented Sep 14, 2023 •

edited

dergoegge commented Sep 14, 2023 •

edited

instagibbs Sep 15, 2023 •

edited