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

theStack commented at 2:42 pm on September 1, 2024: contributor

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: https://github.com/bitcoin/bitcoin/blob/5abb9b1af49be9024f95fa2f777285c531785d85/src/txorphanage.cpp#L22-L34 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 commented at 2:42 pm on September 1, 2024: contributor

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.

DrahtBot added the label Tests on Sep 1, 2024

in test/functional/p2p_orphan_handling.py:577 in 30aab71063 outdated

572+    def test_orphan_reject_large_tx(self):
573+        self.log.info("Test that large transactions (>MAX_STANDARD_TX_WEIGHT) aren't put into orphanage")
574+        # we have to explicitly allow non-standard txs to reach this code path, as otherwise
575+        # the tx is already rejected earlier with "tx-size" (see `IsStandardTx` check)
576+        self.restart_node(0, extra_args=['-acceptnonstdtxn=1'])
577+        oversize_weight = MAX_STANDARD_TX_WEIGHT + 1

tdb3 commented at 6:48 pm on September 1, 2024:

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

tdb3 approved

tdb3 commented at 7:00 pm on September 1, 2024: contributor

ACK 30aab71063d039d4386f580a031cb5e5e742c363

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 commented at 10:54 pm on September 2, 2024: contributor

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 approved

danielabrozzoni commented at 12:35 pm on September 3, 2024: contributor

tACK 30aab71063d039d4386f580a031cb5e5e742c363

fanquake requested review from glozow on Sep 3, 2024

glozow commented at 2:46 pm on September 3, 2024: member

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.

test: add `BulkTransaction` helper to unit test transaction utils

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

ed7d224666

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

theStack force-pushed on Sep 3, 2024

theStack commented at 8:45 pm on September 3, 2024: contributor

@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 commented at 8:56 pm on September 3, 2024: member

ACK 66d13c870284327abc89d36c0b5cc5f58e96f570

DrahtBot requested review from tdb3 on Sep 3, 2024

DrahtBot requested review from danielabrozzoni on Sep 3, 2024

tdb3 approved

tdb3 commented at 11:08 pm on September 3, 2024: contributor

re-ACK 66d13c870284327abc89d36c0b5cc5f58e96f570

Nice unit test addition.

maflcko commented at 1:44 pm on September 4, 2024: member

review ACK 66d13c870284327abc89d36c0b5cc5f58e96f570

glozow merged this on Sep 4, 2024

glozow closed this on Sep 4, 2024

theStack deleted the branch on Sep 4, 2024

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

Code Coverage

Reviews

Conflicts