phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-10 03:47:29 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Merge bitcoin/bitcoin#29424: v3 followups

Browse source 
6b161cb82a [test] second child of a v3 tx can be replaced individually (glozow)
5c998a696c [refactor] use MAX_PUBKEYS_PER_MULTISIG instead of magic numbers in test (glozow)
a9346421db [test] PackageV3Checks with inheritance violation in mempool ancestor (glozow)
63b62e123e [doc] fix docs and comments from v3 (glozow)

Pull request description:

  Addresses final comments from #28948:
  - thread at https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483245289, using 87fc7f0a8d with some modifications
  - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483769698
  - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1483776227
  - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484427635
  - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484467280
  - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484531064
  - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484992098
  - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484992336
  - https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1484994642

ACKs for top commit:
  instagibbs:
    ACK 6b161cb82a
  sdaftuar:
    utACK 6b161cb82a

Tree-SHA512: 584fce7810f4d704ee6ab51fdc7d42bab342140cae3d076f89b5e1966dd1dd8293cb25b3121e41a4dcd65f9d4a735102b9ab2e90f98aa770b84e21f4d35d63d3
This commit is contained in:
fanquake 2024-02-13 09:46:13 -03:00
commit 37fdf5a492
No known key found for this signature in database
GPG key ID: 2EEB9F5CC09526C1
6 changed files with 44 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
doc/policy/mempool-replacements.md
View file

@ -12,7 +12,7 @@ other consensus and policy rules, each of the following conditions are met:
1. The directly conflicting transactions all signal replaceability explicitly. A transaction is
signaling BIP125 replaceability if any of its inputs have an nSequence number less than (0xffffffff - 1).
A transaction also signals replaceibility if its nVersion field is set to 3.
A transaction also signals replaceability if its nVersion field is set to 3.
*Rationale*: See [BIP125
explanation](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki#motivation).

5
src/policy/v3_policy.cpp
View file

@ -81,9 +81,9 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
vsize, V3_CHILD_MAX_VSIZE);
}
// Exactly 1 parent exists, either in mempool or package. Find it.
const auto parent_info = [&] {
if (mempool_ancestors.size() > 0) {
// There's a parent in the mempool.
auto& mempool_parent = *mempool_ancestors.begin();
Assume(mempool_parent->GetCountWithDescendants() == 1);
return ParentInfo{mempool_parent->GetTx().GetHash(),
@ -91,7 +91,6 @@ std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t v
mempool_parent->GetTx().nVersion,
/*has_mempool_descendant=*/mempool_parent->GetCountWithDescendants() > 1};
} else {
// Ancestor must be in the package. Find it.
auto& parent_index = in_package_parents.front();
auto& package_parent = package.at(parent_index);
return ParentInfo{package_parent->GetHash(),
@ -184,7 +183,7 @@ std::optional<std::string> SingleV3Checks(const CTransactionRef& ptx,
// The rest of the rules only apply to transactions with nVersion=3.
if (ptx->nVersion != 3) return std::nullopt;
// Check that V3_ANCESTOR_LIMIT would not be violated, including both in-package and in-mempool.
// Check that V3_ANCESTOR_LIMIT would not be violated.
if (mempool_ancestors.size() + 1 > V3_ANCESTOR_LIMIT) {
return strprintf("tx %s (wtxid=%s) would have too many ancestors",
ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString());

2
src/test/fuzz/tx_pool.cpp
View file

@ -407,8 +407,6 @@ FUZZ_TARGET(tx_pool, .init = initialize_tx_pool)
const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID;
if (accepted) {
txids.push_back(tx->GetHash());
// Only check fees if accepted and not bypass_limits, otherwise it's not guaranteed that
// trimming has happened for this tx and previous iterations.
CheckMempoolV3Invariants(tx_pool);
}
}

4
src/test/txvalidation_tests.cpp
View file

@ -119,6 +119,8 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup)
Package package_v3_v2{mempool_tx_v3, tx_v2_from_v3};
BOOST_CHECK_EQUAL(*PackageV3Checks(tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), package_v3_v2, empty_ancestors), expected_error_str);
CTxMemPool::setEntries entries_mempool_v3{pool.GetIter(mempool_tx_v3->GetHash().ToUint256()).value()};
BOOST_CHECK_EQUAL(*PackageV3Checks(tx_v2_from_v3, GetVirtualTransactionSize(*tx_v2_from_v3), {tx_v2_from_v3}, entries_mempool_v3), expected_error_str);
// mempool_tx_v3 mempool_tx_v2
// ^ ^
@ -149,6 +151,8 @@ BOOST_FIXTURE_TEST_CASE(version3_tests, RegTestingSetup)
Package package_v2_v3{mempool_tx_v2, tx_v3_from_v2};
BOOST_CHECK_EQUAL(*PackageV3Checks(tx_v3_from_v2, GetVirtualTransactionSize(*tx_v3_from_v2), package_v2_v3, empty_ancestors), expected_error_str);
CTxMemPool::setEntries entries_mempool_v2{pool.GetIter(mempool_tx_v2->GetHash().ToUint256()).value()};
BOOST_CHECK_EQUAL(*PackageV3Checks(tx_v3_from_v2, GetVirtualTransactionSize(*tx_v3_from_v2), {tx_v3_from_v2}, entries_mempool_v2), expected_error_str);
// mempool_tx_v3 mempool_tx_v2
// ^ ^

33
test/functional/mempool_accept_v3.py
View file

@ -2,6 +2,7 @@
# Copyright (c) 2024 The Bitcoin Core developers
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
from decimal import Decimal
from test_framework.messages import (
MAX_BIP125_RBF_SEQUENCE,
@ -397,6 +398,37 @@ class MempoolAcceptV3(BitcoinTestFramework):
test_accept_2children_with_in_mempool_parent = node.testmempoolaccept([tx_v3_child_1["hex"], tx_v3_child_2["hex"]])
assert all([result["package-error"] == expected_error_extra for result in test_accept_2children_with_in_mempool_parent])
@cleanup(extra_args=["-acceptnonstdtxn=1"])
def test_reorg_2child_rbf(self):
node = self.nodes[0]
self.log.info("Test that children of a v3 transaction can be replaced individually, even if there are multiple due to reorg")
ancestor_tx = self.wallet.send_self_transfer_multi(from_node=node, num_outputs=2, version=3)
self.check_mempool([ancestor_tx["txid"]])
block = self.generate(node, 1)[0]
self.check_mempool([])
child_1 = self.wallet.send_self_transfer(from_node=node, version=3, utxo_to_spend=ancestor_tx["new_utxos"][0])
child_2 = self.wallet.send_self_transfer(from_node=node, version=3, utxo_to_spend=ancestor_tx["new_utxos"][1])
self.check_mempool([child_1["txid"], child_2["txid"]])
self.generate(node, 1)
self.check_mempool([])
# Create a reorg, causing ancestor_tx to exceed the 1-child limit
node.invalidateblock(block)
self.check_mempool([ancestor_tx["txid"], child_1["txid"], child_2["txid"]])
assert_equal(node.getmempoolentry(ancestor_tx["txid"])["descendantcount"], 3)
# Create a replacement of child_1. It does not conflict with child_2.
child_1_conflict = self.wallet.send_self_transfer(from_node=node, version=3, utxo_to_spend=ancestor_tx["new_utxos"][0], fee_rate=Decimal("0.01"))
# Ensure child_1 and child_1_conflict are different transactions
assert (child_1_conflict["txid"] != child_1["txid"])
self.check_mempool([ancestor_tx["txid"], child_1_conflict["txid"], child_2["txid"]])
assert_equal(node.getmempoolentry(ancestor_tx["txid"])["descendantcount"], 3)
def run_test(self):
self.log.info("Generate blocks to create UTXOs")
node = self.nodes[0]
@ -412,6 +444,7 @@ class MempoolAcceptV3(BitcoinTestFramework):
self.test_mempool_sibling()
self.test_v3_package_inheritance()
self.test_v3_in_testmempoolaccept()
self.test_reorg_2child_rbf()
if __name__ == "__main__":

8
test/functional/mempool_sigoplimit.py
View file

@ -39,7 +39,7 @@ from test_framework.wallet import MiniWallet
from test_framework.wallet_util import generate_keypair
DEFAULT_BYTES_PER_SIGOP = 20 # default setting
MAX_PUBKEYS_PER_MULTISIG = 20
class BytesPerSigOpTest(BitcoinTestFramework):
def set_test_params(self):
@ -159,13 +159,13 @@ class BytesPerSigOpTest(BitcoinTestFramework):
# Separately, the parent tx is ok
parent_individual_testres = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex()])[0]
assert parent_individual_testres["allowed"]
# Multisig is counted as MAX_PUBKEYS_PER_MULTISIG = 20 sigops
assert_equal(parent_individual_testres["vsize"], 5000 * 20)
max_multisig_vsize = MAX_PUBKEYS_PER_MULTISIG * 5000
assert_equal(parent_individual_testres["vsize"], max_multisig_vsize)
# But together, it's exceeding limits in the *package* context. If sigops adjusted vsize wasn't being checked
# here, it would get further in validation and give too-long-mempool-chain error instead.
packet_test = self.nodes[0].testmempoolaccept([tx_parent.serialize().hex(), tx_child.serialize().hex()])
expected_package_error = f"package-mempool-limits, package size {2*20*5000} exceeds ancestor size limit [limit: 101000]"
expected_package_error = f"package-mempool-limits, package size {2*max_multisig_vsize} exceeds ancestor size limit [limit: 101000]"
assert_equal([x["package-error"] for x in packet_test], [expected_package_error] * 2)
# When we actually try to submit, the parent makes it into the mempool, but the child would exceed ancestor vsize limits