-
Notifications
You must be signed in to change notification settings - Fork 36.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: Extend test coverage of BIP125 and document confusing/inconsistent behavior #22867
Conversation
Concept ACK, thanks for adding tests |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
84c88ec
to
85a69f4
Compare
Rebased |
Concept ACK. Tested on macOS Big Sur. |
85a69f4
to
e6c88a3
Compare
Addressed @pg156's comments/improvements and rebased. |
e6c88a3
to
a9a9d3f
Compare
a9a9d3f
to
12509a0
Compare
Rebased |
12509a0
to
3fc1f8b
Compare
I better organized the commits and improved the commit messages. Very minor / no behavior change |
In the event of a reorg, the assumption that a newly added tx has no in-mempool children is false. If the children opted-out of rbf they would show \`RBFTransactionState::FINAL\` prior to the reorg. Upon their rbf opt-in parent being accepted back into the mempool, they would show \`RBFTransactionState::REPLACEABLE_BIP125\`.
Highlight that transactions may signal they are replaceable even though they are not due to BIP125 RBF (Rule bitcoin#5).
3fc1f8b
to
64c2040
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK 64c2040
Great job, these tests really clarify the current behavior of rbf.
fee_rate=Decimal('0.0001'), | ||
) | ||
assert_equal(True, self.nodes[0].getmempoolentry(tx['txid'])['bip125-replaceable']) # inherited | ||
# Now we have a chain of: `optin_parent_tx`, `joined_tx`, and 99 txs. The last tx in the loop exceeded `MAX_REPLACEMENT_LIMIT` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: My understanding is that it's better not to have actual values in comments
# Now we have a chain of: `optin_parent_tx`, `joined_tx`, and 99 txs. The last tx in the loop exceeded `MAX_REPLACEMENT_LIMIT` | |
# Now we have a chain of: `optin_parent_tx`, `joined_tx`, and `MAX_REPLACEMENT_LIMIT - 1` txs. The last tx in the loop exceeded `MAX_REPLACEMENT_LIMIT` |
assert_equal(True, self.nodes[0].getmempoolentry(tx["txid"])['bip125-replaceable']) | ||
|
||
# Case in point, we can't actually replace `optin_parent_tx` once it has `MAX_REPLACEMENT_LIMIT` descendants | ||
self.wallet.create_self_transfer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: To check the specific error message
self.wallet.create_self_transfer( | |
replacement_parent_tx = self.wallet.create_self_transfer( | |
from_node=self.nodes[0], | |
utxo_to_spend=confirmed_utxos[0], | |
fee_rate=Decimal('0.01'), | |
mempool_valid=False | |
)['hex'] | |
assert_raises_rpc_error(-26, 'too many potential replacements', self.nodes[0].sendrawtransaction, replacement_parent_tx, 0) |
Concept ACK. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making an effort to clarify the RBF interface, but I believe that clear documentation is more appropriate than a functional test for 2/4 of these.
@@ -621,5 +637,64 @@ def test_replacement_relay_fee(self): | |||
tx.vout[0].nValue -= 1 | |||
assert_raises_rpc_error(-26, "insufficient fee", self.nodes[0].sendrawtransaction, tx.serialize().hex()) | |||
|
|||
def test_prechecks_overestimates_replacements(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit 5e72716 "test: replacement tx rejected bc conflicts may be double counted." can be dropped
I assume the purpose of this test is to illustrate the limitation/overestimation in GetEntriesForConflicts()
. It is now documented here. If we want to test this, as it's a low-level implementation detail, a unit test would be more appropriate than a functional test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's documented here is not such an important issue as the CVE in test_no_inherited_signaling
but documenting an issue/limitation seems to have a precedent. Maybe this could make sense as an additional reference for the docs?
@@ -705,7 +705,7 @@ def test_prechecks_overestimates_replacements(self): | |||
self.wallet.get_utxo(txid=tx['txid']) | |||
self.wallet.get_utxo(txid=replacement_tx['txid']) | |||
|
|||
def test_reorged_inherited_signaling(self): | |||
def test_reorged_inherited_signaling_and_descendant_limit(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit 64c2040 "test: incorrect rbf status when max replacement limit exceeded"
I think this one is also a misunderstanding and can be dropped/replaced with a clarification in the RPC helpstring. The bip125-replaceable
field is only concerned with signaling is not an authority on all RBF rules, so Rule 5 shouldn't affect its result. If that isn't clear to users, we should more clearly document what information it provides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 64c2040 (make check
and test/functional/test_runner.py
).
Tested on NixOS 22.05 64 bits.
test_prechecks_overestimates_replacements
I commented the following lines locally and the test failed accordingly.
Lines 64 to 69 in 64c2040
if (nConflictingCount > MAX_BIP125_REPLACEMENT_CANDIDATES) { | |
return strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", | |
txid.ToString(), | |
nConflictingCount, | |
MAX_BIP125_REPLACEMENT_CANDIDATES); | |
} |
Increasing the following constant to 104 or more makes the test fail as expected because there would be enough room in the mempool to fit the chain of 104 transactions : parent_txs (2) + joined_tx (2) + tx chain (100 / 2 but UTXOs counted twice so 100) = 104
.
Line 17 in 64c2040
static constexpr uint32_t MAX_BIP125_REPLACEMENT_CANDIDATES{100}; |
test_reorged_inherited_signaling_and_descendant_limit
Changing the following line
Line 34 in 64c2040
pool.CalculateMemPoolAncestors(entry, ancestors, noLimit, noLimit, noLimit, noLimit, dummy, false); |
to
pool.CalculateMemPoolAncestors(entry, ancestors, MAX_BIP125_REPLACEMENT_CANDIDATES, noLimit, noLimit, noLimit, dummy, false);
results in the test failing as expected. The last transaction of the transaction chain of MAX_REPLACEMENT_LIMIT size would stop signaling RBF with inheritance because the child transaction signaling RBF would be buried too deep in the chain to be detected: which is to my understanding not the current behavior of Bitcoin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
joined_tx_txid = self.nodes[0].sendrawtransaction(joined_tx_hex) | ||
|
||
# Get the `joined_tx` utxo into our wallet so we can spend a chain of descendants from it | ||
joined_tx = self.nodes[0].decoderawtransaction(joined_tx_hex) | ||
self.wallet.scan_tx(joined_tx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
joined_tx_txid = self.nodes[0].sendrawtransaction(joined_tx_hex) | |
# Get the `joined_tx` utxo into our wallet so we can spend a chain of descendants from it | |
joined_tx = self.nodes[0].decoderawtransaction(joined_tx_hex) | |
self.wallet.scan_tx(joined_tx) | |
joined_tx_txid = self.wallet.sendrawtransaction(from_node=self.nodes[0], tx_hex=joined_tx_hex) |
# Even though there are well under `MAX_REPLACEMENT_LIMIT` transactions that will be evicted due to this replacement, | ||
# in this case we still reject the replacement attempt because of the way `MemPoolAccept::PreChecks` double-counts descendants. | ||
# Each `confirmed_utxo` has the exact same descendants, but they are each counted twice! | ||
replacement_attempt_tx_hex = self.create_double_input_self_transfer(confirmed_utxos, Decimal('0.01')) | ||
assert_raises_rpc_error(-26, 'too many potential replacements', self.nodes[0].sendrawtransaction, replacement_attempt_tx_hex, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are doing good job documenting this limitation, you could maybe add one or two getmempoolinfo()["size"])
assertions at appropriate places to help even more with the overall explanation.
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
Closing as I believe this is resolved with doc/policy/mempool_replacements.md (which documents the limitations) and #25674, and this has needed rebase for a while. |
Intention of this PR is to extend test coverage of BIP125. Mostly around Rule 5. Focus is on highlighting confusing/inconsistent behavior that I think we need to consider as we move forward with multiple PRs (and different approaches to BIP125 in general) that are under review.
This PR also splits tests out of #22698 (which I've made a draft for now until some related PRs are merged) as they are generally useful and highlight some behavior I think is worth considering.
All that said, I do not see any reason we can't eliminate all these inconsistencies while keeping inherited signaling, which is what #22698 does. But either way, making it clear in tests at least motivates a longer-term fix