test: add check that too large txs aren't put into orphanage #30784

theStack · 2024-09-01T14:42:43Z

This PR adds test coverage for the following check in TxOrphanage::AddTx, where large orphan txs are ignored in order to avoid memory exhaustion attacks:

bitcoin/src/txorphanage.cpp

Lines 22 to 34 in 5abb9b1

    
           // Ignore big transactions, to avoid a 
        
           // send-big-orphans memory exhaustion attack. If a peer has a legitimate 
        
           // large transaction with a missing parent then we assume 
        
           // it will rebroadcast it later, after the parent transaction(s) 
        
           // have been mined or received. 
        
           // 100 orphans, each of which is at most 100,000 bytes big is 
        
           // at most 10 megabytes of orphans and somewhat more byprev index (in the worst case): 
        
           unsigned int sz = GetTransactionWeight(*tx); 
        
           if (sz > MAX_STANDARD_TX_WEIGHT) 
        
           { 
        
               LogPrint(BCLog::TXPACKAGES, "ignoring large orphan tx (size: %u, txid: %s, wtxid: %s)\n", sz, hash.ToString(), wtxid.ToString()); 
        
               return false; 
        
           }

Note that this code-path isn't reachable under normal circumstances, as txs larger than MAX_STANDARD_TX_WEIGHT are already rejected earlier in the course of doing the mempool standardness checks (see MemPoolAccept::PreChecks -> IsStandardTx -> reason = "tx-size";), so this is only relevant if tx standardness rules are disabled via -acceptnonstdtxns=1. The ignore path is checked ~~by asserting the debug log, which is ugly, but as far as I know there is currently no way to access the orphanage entries from the outside~~ via unit test that checks the return value of AddTx. As an alternative to adding test coverage, one might consider removing this check altogether (or replacing it with an Assume), as it's redundant as explained above.

DrahtBot · 2024-09-01T14:42:46Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, tdb3, maflcko
Stale ACK	danielabrozzoni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30561 (refactor: move SignSignature helpers to test utils by theStack)

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.

tdb3

ACK 30aab71

Nice test addition. Worked well.

The ignore path is checked by asserting the debug log, which is ugly, but as far as I know there is currently no way to access the orphanage entries from the outside. As an alternative to adding test coverage, one might consider removing this check altogether (or replacing it with an Assume), as it's redundant as explained above.

RPC already has a way to get mempool transactions (e.g. getrawmempool and getmempoolentry), but as far as I can tell, not orphans. Unless I'm overlooking something (or there's a history I'm not aware of), might be nice to have a getorphantxs (returning orphan txids, etc.) and getorphan "txid" (details for an orphan). Orphans do expire relatively quickly, so results from these RPC would be pretty ephemeral, but it might still be worth knowing. I'm taking a look at implementation options.

An added bonus would be that p2p_orphan_handling could call getorphantxs directly (rather than search the debug log). If others agree that these new RPCs are worthwhile, then the RPCs should be implemented in a separate/followup PR, and in my opinion shouldn't impact this PR getting merged (an adjustment of the test could be done later).

tdb3 · 2024-09-01T18:48:24Z

test/functional/p2p_orphan_handling.py

+        # we have to explicitly allow non-standard txs to reach this code path, as otherwise
+        # the tx is already rejected earlier with "tx-size" (see `IsStandardTx` check)
+        self.restart_node(0, extra_args=['-acceptnonstdtxn=1'])
+        oversize_weight = MAX_STANDARD_TX_WEIGHT + 1


Locally modified this (to 1000) to see the assert_debug_log check fail (missing larg_orphan_logmsg). The assert failed as expected.

tdb3 · 2024-09-02T22:54:03Z

RPC already has a way to get mempool transactions (e.g. getrawmempool and getmempoolentry), but as far as I can tell, not orphans. Unless I'm overlooking something (or there's a history I'm not aware of), might be nice to have a getorphantxs (returning orphan txids, etc.) and getorphan "txid" (details for an orphan). Orphans do expire relatively quickly, so results from these RPC would be pretty ephemeral, but it might still be worth knowing. I'm taking a look at implementation options.

Draft PR opened #30793

danielabrozzoni

tACK 30aab71

glozow

Concept ACK, but this should be a unit test, not a functional test looking for logs. We could accidentally delete the return false; line but keep the LogPrint line, and this test would pass.

so this is only relevant if tx standardness rules are disabled via -acceptnonstdtxns=1.

Similarly, it's a bit icky to have to disable standardness checks to test this.

A unit test in orphanage_tests.cpp could directly create a large tx and call AddTx. With #30110, we can also test by calling MempoolRejectedTx with a large transaction and TX_MISSING_INPUTS.

The harder part there is adding a utility function to bulk transactions in our unit testing framework, but I think that would be quite helpful in the long run, so I'd gladly review that.

The padding method used matches the one used in MiniWallet, `MiniWallet._bulk_tx`.

theStack · 2024-09-03T20:45:46Z

@glozow:

Concept ACK, but this should be a unit test, not a functional test looking for logs. We could accidentally delete the return false; line but keep the LogPrint line, and this test would pass.

so this is only relevant if tx standardness rules are disabled via -acceptnonstdtxns=1.

Similarly, it's a bit icky to have to disable standardness checks to test this.

Agree! For some reason I haven't even considered doing this as a unit test, but it makes much more sense indeed.

A unit test in orphanage_tests.cpp could directly create a large tx and call AddTx. With #30110, we can also test by calling MempoolRejectedTx with a large transaction and TX_MISSING_INPUTS.

Done as suggested. (For reference, the functional test branch is still available at https://github.com/theStack/bitcoin/tree/202409-test-orphanage_large_tx_rejection-functional)

The harder part there is adding a utility function to bulk transactions in our unit testing framework, but I think that would be quite helpful in the long run, so I'd gladly review that.

Added a new helper BulkTransaction which matches the implementation of MiniWallet's ._bulk_tx method, so it should be quite straight-forward to review. Note that I still think for most use-cases vsize would be a better padding unit to simplify the call-sites w.r.t. policy checks where weight is usually not used (see #30718), but I'll just keep it in sync with MiniWallet here.

glozow

ACK 66d13c8

tdb3

re-ACK 66d13c8

Nice unit test addition.

maflcko · 2024-09-04T13:44:29Z

review ACK 66d13c8

DrahtBot added the Tests label Sep 1, 2024

tdb3 approved these changes Sep 1, 2024

View reviewed changes

tdb3 mentioned this pull request Sep 2, 2024

rpc: add getorphantxs #30793

Merged

danielabrozzoni approved these changes Sep 3, 2024

View reviewed changes

fanquake requested a review from glozow September 3, 2024 12:41

glozow reviewed Sep 3, 2024

View reviewed changes

theStack added 2 commits September 3, 2024 22:20

test: add BulkTransaction helper to unit test transaction utils

ed7d224

The padding method used matches the one used in MiniWallet, `MiniWallet._bulk_tx`.

test: add check that large txs aren't put into orphanage

66d13c8

theStack force-pushed the 202409-test-orphanage_large_tx_rejection branch from 30aab71 to 66d13c8 Compare September 3, 2024 20:28

glozow reviewed Sep 3, 2024

View reviewed changes

DrahtBot requested review from tdb3 and danielabrozzoni September 3, 2024 20:56

tdb3 approved these changes Sep 3, 2024

View reviewed changes

DrahtBot mentioned this pull request Sep 4, 2024

refactor: move SignSignature helpers to test utils #30561

Merged

glozow merged commit f66011e into bitcoin:master Sep 4, 2024
16 checks passed

theStack deleted the 202409-test-orphanage_large_tx_rejection branch September 4, 2024 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test: add check that too large txs aren't put into orphanage #30784

test: add check that too large txs aren't put into orphanage #30784

Uh oh!

theStack commented Sep 1, 2024 •

edited

Loading

Uh oh!

DrahtBot commented Sep 1, 2024 •

edited

Loading

Uh oh!

tdb3 left a comment •

edited

Loading

Uh oh!

tdb3 Sep 1, 2024

Uh oh!

tdb3 commented Sep 2, 2024

Uh oh!

danielabrozzoni left a comment

Uh oh!

glozow left a comment

Uh oh!

theStack commented Sep 3, 2024 •

edited

Loading

Uh oh!

glozow left a comment

Uh oh!

tdb3 left a comment

Uh oh!

maflcko commented Sep 4, 2024

Uh oh!

Uh oh!

Uh oh!

	// Ignore big transactions, to avoid a
	// send-big-orphans memory exhaustion attack. If a peer has a legitimate
	// large transaction with a missing parent then we assume
	// it will rebroadcast it later, after the parent transaction(s)
	// have been mined or received.
	// 100 orphans, each of which is at most 100,000 bytes big is
	// at most 10 megabytes of orphans and somewhat more byprev index (in the worst case):
	unsigned int sz = GetTransactionWeight(*tx);
	if (sz > MAX_STANDARD_TX_WEIGHT)
	{
	LogPrint(BCLog::TXPACKAGES, "ignoring large orphan tx (size: %u, txid: %s, wtxid: %s)\n", sz, hash.ToString(), wtxid.ToString());
	return false;
	}

test: add check that too large txs aren't put into orphanage #30784

test: add check that too large txs aren't put into orphanage #30784

Uh oh!

Conversation

theStack commented Sep 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

tdb3 left a comment • 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!

tdb3 Sep 1, 2024

Choose a reason for hiding this comment

Uh oh!

tdb3 commented Sep 2, 2024

Uh oh!

danielabrozzoni left a comment

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Sep 4, 2024

Uh oh!

Uh oh!

Uh oh!

theStack commented Sep 1, 2024 •

edited

Loading

DrahtBot commented Sep 1, 2024 •

edited

Loading

tdb3 left a comment •

edited

Loading

theStack commented Sep 3, 2024 •

edited

Loading