After joining the bitcoin pr review club about #30793
I learned about CVE-2012-3789
So I was motivated to write a functional test that covers this part of the code,
This test should add the max number of orphans to a nodes orphanage and then attempt to add another, then asserts that the number of orphans is still at the max amount
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | rkrux, instagibbs, tdb3, achow101 |
| Concept ACK | Prabhat1308 |
| Stale ACK | Christewart |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
Concept ACK
Thanks, this is useful!
I’m uncertain if this check is better suited for p2p_orphan_handling or rpc_getorphantxs (or for a feature_orphanage or mempool_orphanage). If p2p_orphan_handling is the preferred location, I’m happy to include your commit (preserving you as author) in #31037.
https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#naming-guidelines
Can also check unit tests (https://github.com/bitcoin/bitcoin/blob/master/src/test/orphanage_tests.cpp)
147+ self.log.info("Check that we do not add more than the max orphan amount and that the first orphan has been dropped")
148+ tx_parent_1 = self.wallet.create_self_transfer()
149+ tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"])
150+ peer_1.send_and_ping(msg_tx(tx_child_1["tx"]))
151+ orphanage = node.getorphantxs(verbosity=2)
152+ assert tx_in_orphanage(node, tx_child_1["tx"])
TxOrphanage::LimitOrphans is chosen randomly, i.e. it could also be the last one added (tx_child_1 in this case). Didn’t test locally yet, but I’d assume that about every 100th test run fails.
Concept ACK Thanks, this is useful! I’m uncertain if this check is better suited for
p2p_orphan_handlingorrpc_getorphantxs(or for afeature_orphanageormempool_orphanage). Ifp2p_orphan_handlingis the preferred location, I’m happy to include your commit (preserving you as author) in #31037.https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#naming-guidelines
Can also check unit tests (https://github.com/bitcoin/bitcoin/blob/master/src/test/orphanage_tests.cpp)
yes if you would like to pick this commit that would be great then I can close this PR
137+ for i in range(MAX_ORPHANS):
138+ tx_parent_1 = self.wallet.create_self_transfer()
139+ tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"])
140+ peer_1.send_and_ping(msg_tx(tx_child_1["tx"]))
141+
142+ orphanage = node.getorphantxs(verbosity=2)
orphanage anywhere
139+ tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"])
140+ peer_1.send_and_ping(msg_tx(tx_child_1["tx"]))
141+
142+ orphanage = node.getorphantxs(verbosity=2)
143+ assert tx_in_orphanage(node, tx_child_1["tx"])
144+ orphanage = node.getorphantxs(verbosity=2)
verbosity=2 is needed
146+
147+ self.log.info("Check that we do not add more than the max orphan amount and that the first orphan has been dropped")
148+ tx_parent_1 = self.wallet.create_self_transfer()
149+ tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"])
150+ peer_1.send_and_ping(msg_tx(tx_child_1["tx"]))
151+ orphanage = node.getorphantxs(verbosity=2)
135+
136+ self.log.info("Filling up orphanage with " + str(MAX_ORPHANS) + "(MAX_ORPHANS) orphans")
137+ for i in range(MAX_ORPHANS):
138+ tx_parent_1 = self.wallet.create_self_transfer()
139+ tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"])
140+ peer_1.send_and_ping(msg_tx(tx_child_1["tx"]))
The test is currently a bit slow, and I think it might be because you’re using send_and_ping, which waits for the other peer to send back a pong in each iteration of the loop.
You could send the messages in the for loop and then ping only at the end:
0 for i in range(MAX_ORPHANS):
1 tx_parent_1 = self.wallet.create_self_transfer()
2 tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"])
3 peer_1.send_message(msg_tx(tx_child_1["tx"]))
4
5 peer_1.sync_with_ping()
6
7 orphanage = node.getorphantxs()
8 assert_equal(len(orphanage), MAX_ORPHANS)
The drawback would be that you’re not checking that every single tx is entering the orphanage, although you could do that simply by saving the orphans and checking after the ping:
0 orphans = []
1 for i in range(MAX_ORPHANS):
2 tx_parent_1 = self.wallet.create_self_transfer()
3 tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"])
4 orphans.append(tx_child_1["tx"])
5 peer_1.send_message(msg_tx(tx_child_1["tx"]))
6
7 peer_1.sync_with_ping()
8
9 orphanage = node.getorphantxs()
10 assert_equal(len(orphanage), MAX_ORPHANS)
11
12 for orphan in orphans:
13 assert tx_in_orphanage(node, orphan)
Personally I think the first option is good enough though, what do you think?
Thank you for the review! I’ve updated in 4d59b9a
the second one looked good to me! I prefer asserting that each individual tx made it into the orphanage
133+
134+ peer_1 = node.add_p2p_connection(P2PInterface())
135+
136+ self.log.info("Filling up orphanage with " + str(MAX_ORPHANS) + "(MAX_ORPHANS) orphans")
137+ orphans = []
138+ for i in range(MAX_ORPHANS):
nit
0 for _ in range(MAX_ORPHANS):
146+ assert_equal(len(orphanage), MAX_ORPHANS)
147+
148+ for orphan in orphans:
149+ assert tx_in_orphanage(node, orphan)
150+
151+ self.log.info("Check that we do not add more than the max orphan amount and that the first orphan has been dropped")
Thanks! I updated in 60dbd42
Its supposed to be a random orphan but I removed “and that the first orphan has been dropped” since we are not checking if a random orphan has been dropped and just checking the max orphan amount
I’m a bit confused to the behavior i’m observing on this PR. Not necessarily anything to do with the test, but how orphans are handled.
I tried adding another part to the test case to check we can clear the orphanage by propagating parent transactions: https://github.com/Christewart/bitcoin/commit/04815c463d331f674f67fdcb3b8da601484d7a33
It appears that to totally clear the orphanage, we need to propagate the parent transaction for the last child transaction (the orphan we rejected because we surpassed MAX_ORPHANS), which is unexpected I believe?
I’m a bit confused to the behavior i’m observing on this PR. Not necessarily anything to do with the test, but how orphans are handled.
I tried adding another part to the test case to check we can clear the orphanage by propagating parent transactions: Christewart@04815c4
It appears that to totally clear the orphanage, we need to propagate the parent transaction for the last child transaction (the orphan we rejected because we surpassed
MAX_ORPHANS), which is unexpected I believe?
ok figured this out, it appears we have 1 orphan in the mempool when the unit test begins. I’ve modified the code to account for this: https://github.com/Christewart/bitcoin/commit/712206ead880472fcc64c330bee602f29c03f1cb
I’m a bit confused to the behavior i’m observing on this PR. Not necessarily anything to do with the test, but how orphans are handled. I tried adding another part to the test case to check we can clear the orphanage by propagating parent transactions: Christewart@04815c4 It appears that to totally clear the orphanage, we need to propagate the parent transaction for the last child transaction (the orphan we rejected because we surpassed
MAX_ORPHANS), which is unexpected I believe?ok figured this out, it appears we have 1 orphan in the mempool when the unit test begins. I’ve modified the code to account for this: Christewart@712206e
Thank you for the review!
I updated the PR in bf46723
I restarted the node with -persistmempool=0 so we drop that initial orphan in the mempool and then also I added the additional code you added for clearing the orphanage since that seemed like useful cleanup in the test
Concept ACK for the test While we are trying to add another orphan transaction when the max amount is reached , why are we dropping a random orphan and not just the incoming orphan ? Even in the condition of the node getting DOSed , having to remove a random transaction translates to some amount of work done while its not the case when we just drop the incoming transaction.
If we drop the incoming orphan then an attacker can quickly fill up a node’s orphan pool with tx’s that will never get a valid parent and then effectively block honest child transactions which are awaiting their parent
18@@ -19,6 +19,7 @@
19 MiniWallet,
20 )
21
22+MAX_ORPHANS = 100
tACK bf46723537c37cb1ee7f84ffe7b75c253beadb89
Successful make and functional tests.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/31946723457
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
126@@ -125,6 +127,48 @@ def orphan_details_match(self, orphan, tx, verbosity):
127 self.log.info("Check the transaction hex of orphan")
128 assert_equal(orphan["hex"], tx["hex"])
129
130+ def test_max_orphan_amount(self):
131+ self.log.info("Check that once we reach the max orphan amount we dont get any new orphans")
We accept new orphans, we just never exceed limits
0 self.log.info("Check that we never exceed our storage limits for orphans")
126@@ -125,6 +127,48 @@ def orphan_details_match(self, orphan, tx, verbosity):
127 self.log.info("Check the transaction hex of orphan")
128 assert_equal(orphan["hex"], tx["hex"])
129
130+ def test_max_orphan_amount(self):
131+ self.log.info("Check that once we reach the max orphan amount we dont get any new orphans")
132+
133+ MAX_ORPHANS = 100
134+
135+ self.restart_node(0, extra_args=["-persistmempool=0"])
p2p_orphan_handling.py in 3bc3bcc
141+ self.wait_until(lambda: len(node.getorphantxs()) == 0)
142+
143+ self.log.info("Filling up orphanage with " + str(MAX_ORPHANS) + "(MAX_ORPHANS) orphans")
144+ orphans = []
145+ parent_orphans = []
146+ for _ in range(MAX_ORPHANS):
19@@ -20,6 +20,8 @@ def run_test(self):
20 self.wallet = MiniWallet(self.nodes[0])
21 self.test_orphan_activity()
22 self.test_orphan_details()
23+ self.generate(self.wallet, 160)
MAX_ORPHANS is defined it can be used here directly
ACK 6741cde5fe3575cc0b58a52c4a30058fa08c4beb
non blocking nits only
Concept ACK Thanks, this is useful! I’m uncertain if this check is better suited for
p2p_orphan_handlingorrpc_getorphantxs(or for afeature_orphanageormempool_orphanage). Ifp2p_orphan_handlingis the preferred location, I’m happy to include your commit (preserving you as author) in #31037. https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#naming-guidelines Can also check unit tests (https://github.com/bitcoin/bitcoin/blob/master/src/test/orphanage_tests.cpp)yes if you would like to pick this commit that would be great then I can close this PR
Sorry I’ve been out of pocket lately (travel and illness). There’s been a lot of great review on this PR, so I don’t want to derail it.
The thought is that rpc_getorphanstxs would be scoped to testing the functionality of the getorphantxs RPC itself (i.e. does it return accurate/correct info, etc.) and that orphanage functionality (e.g. what happens when we reach capacity) would go where other orphanage-related functionality tests live (e.g. p2p_orphan_handling). Thoughts @instagibbs? This isn’t a hill I’m willing to die on, but I’m thinking it would make this a bit more organized.
I’m planning to make changes to p2p_orphan_handling in #31037, but it might be a few days before I get to it and I don’t want to hold this PR hostage, so I can always rebase on top of this one if merged.
Concept ACK Thanks, this is useful! I’m uncertain if this check is better suited for
p2p_orphan_handlingorrpc_getorphantxs(or for afeature_orphanageormempool_orphanage). Ifp2p_orphan_handlingis the preferred location, I’m happy to include your commit (preserving you as author) in #31037. https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#naming-guidelines Can also check unit tests (https://github.com/bitcoin/bitcoin/blob/master/src/test/orphanage_tests.cpp)yes if you would like to pick this commit that would be great then I can close this PR
Sorry I’ve been out of pocket lately (travel and illness). There’s been a lot of great review on this PR, so I don’t want to derail it. The thought is that
rpc_getorphanstxswould be scoped to testing the functionality of the getorphantxs RPC itself (i.e. does it return accurate/correct info, etc.) and that orphanage functionality (e.g. what happens when we reach capacity) would go where other orphanage-related functionality tests live (e.g.p2p_orphan_handling). Thoughts @instagibbs? This isn’t a hill I’m willing to die on, but I’m thinking it would make this a bit more organized.I’m planning to make changes to
p2p_orphan_handlingin #31037, but it might be a few days before I get to it and I don’t want to hold this PR hostage, so I can always rebase on top of this one if merged.
Hope you are feeling better @tdb3
I’ve moved the new test to p2p_orphan_handling.py in 3bc3bcc
let me know if you have any other suggestions!
41@@ -41,6 +42,8 @@
42 # for one peer and y seconds for another, use specific values instead.
43 TXREQUEST_TIME_SKIP = NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY + OVERLOADED_PEER_TX_DELAY + 1
44
45+MAX_ORPHANS = 100
test/functional/test_framework/mempool_util.py as DEFAULT_MAX_ORPHAN_TRANSACTIONS to match https://github.com/bitcoin/bitcoin/blob/947f2925d55f363cf1880315d467fc2edac04545/src/net_processing.h#L27
568@@ -566,6 +569,47 @@ def test_orphan_txid_inv(self):
569 assert tx_child["txid"] in node_mempool
570 assert_equal(node.getmempoolentry(tx_child["txid"])["wtxid"], tx_child["wtxid"])
571
572+ @cleanup
573+ def test_max_orphan_amount(self):
574+ self.log.info("Check that we never exceed our storage limits for orphans")
575+
576+ self.generate(self.wallet, 160)
Generating 160 blocks might be overkill. The following generates one and moves node = self.nodes[0] to just under the opening log (to match other functions).
0diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py
1index 0a88b4c9587..5276814a087 100755
2--- a/test/functional/p2p_orphan_handling.py
3+++ b/test/functional/p2p_orphan_handling.py
4@@ -572,9 +572,9 @@ class OrphanHandlingTest(BitcoinTestFramework): [@cleanup](/bitcoin-bitcoin/contributor/cleanup/)
5 def test_max_orphan_amount(self):
6 self.log.info("Check that we never exceed our storage limits for orphans")
7-
8- self.generate(self.wallet, 160)
9 node = self.nodes[0]
10+
11+ self.generate(self.wallet, 1)
12 peer_1 = node.add_p2p_connection(P2PInterface())
13
14 self.log.info("Check that orphanage is empty on start of test")
p2p_orphan_handling! Left some non-blocking recommendations.
576+ self.generate(self.wallet, 160)
577+ node = self.nodes[0]
578+ peer_1 = node.add_p2p_connection(P2PInterface())
579+
580+ self.log.info("Check that orphanage is empty on start of test")
581+ self.wait_until(lambda: len(node.getorphantxs()) == 0)
wait_until instead of an assert?
41@@ -41,6 +42,8 @@
42 # for one peer and y seconds for another, use specific values instead.
43 TXREQUEST_TIME_SKIP = NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY + OVERLOADED_PEER_TX_DELAY + 1
44
45+DEFAULT_MAX_ORPHAN_TRANSACTIONS = 100
601+ tx_parent_1 = self.wallet.create_self_transfer()
602+ tx_child_1 = self.wallet.create_self_transfer(utxo_to_spend=tx_parent_1["new_utxo"])
603+ peer_1.send_and_ping(msg_tx(tx_child_1["tx"]))
604+ parent_orphans.append(tx_parent_1["tx"])
605+ orphanage = node.getorphantxs()
606+ assert_equal(len(orphanage), DEFAULT_MAX_ORPHAN_TRANSACTIONS)
0 assert_equal(len(node.getorphantxs()), DEFAULT_MAX_ORPHAN_TRANSACTIONS)
Storing in another variable is not necessary only for one time usage.
You don’t need to invalidate ACKs only for this though.
ACK 5c299ecafe6f336cffa145d28036b04b87e26712
Thanks for adding this test, it’s easy to follow through.
575+
576+ node = self.nodes[0]
577+ self.generate(self.wallet, 1)
578+ peer_1 = node.add_p2p_connection(P2PInterface())
579+
580+ self.log.info("Check that orphanage is empty on start of test")
ACK 5c299ecafe6f336cffa145d28036b04b87e26712
feel free to ignore nit
ACK 5c299ecafe6f336cffa145d28036b04b87e26712
I would also re-ACK quickly if the changes recommended by @rkrux and @instagibbs were implemented, and DEFAULT_MAX_ORPHAN_TRANSACTIONS moved to test_framework/mempool_util.py
