backport: Merge bitcoin#22568, 21800, (partial)22707 #6064
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vijaydasmp
changed the title
vijaydasmp
force-pushed
the
bp23_99
branch
2 times, most recently
from
9b85a54 to
e5ab16f
Compare
vijaydasmp
changed the title
vijaydasmp
changed the title
|
This pull request has conflicts, please rebase. |
vijaydasmp
force-pushed
the
bp23_99
branch
2 times, most recently
from
0e14cee to
a1b3d55
Compare
vijaydasmp
force-pushed
the
bp23_99
branch
2 times, most recently
from
e0518bf to
2b460db
Compare
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
changed the title
vijaydasmp
changed the title
knst
reviewed
Jul 24, 2024
| @@ -68,8 +68,7 @@ def run_test(self): | |||
| self.log.info("Add unbroadcasted tx to mempool on new node and shutdown") | |||
| unbroadcasted_tx_hash = new_wallet.send_self_transfer(from_node=new_node)['txid'] | |||
| assert unbroadcasted_tx_hash in new_node.getrawmempool() | |||
| mempool = new_node.getrawmempool(True) | |||
| assert mempool[unbroadcasted_tx_hash]['unbroadcast'] | |||
| assert new_node.getmempoolentry(unbroadcasted_tx_hash)['unbroadcast'] | |||
There was a problem hiding this comment.
22707 - partial due to missing changes for test/functional/mempool_package_limits.py (which is introduced by bitcoin#21800)
|
This pull request has conflicts, please rebase. |
|
ci seems flaky passed previously https://gitlab.com/dashpay/dash/-/pipelines/1406967916, now failing again after rebase |
vijaydasmp
force-pushed
the
bp23_99
branch
2 times, most recently
from
4b0debf to
cb0c7eb
Compare
|
Hello @PastaPastaPasta , requesting review |
knst
previously approved these changes
Aug 21, 2024
There was a problem hiding this comment.
utACK cb0c7eb
@vijaydasmp, consider to backport bitcoin#22582 in a new follow-up PR so far as it adds extra tests for bitcoin#21800 from this PR
| signed = node.signrawtransactionwithkey(tx_heavy.serialize().hex(), privkeys, prevtxs) | ||
| return tx_from_hex(signed["hex"]) | ||
| # OP_TRUE | ||
| tx_heavy.vin[0].scriptSig = CScript([CScript([OP_TRUE])]) |
There was a problem hiding this comment.
nit: is it typo?
Should it be:
tx_heavy.vin[0].scriptSig = CScript([OP_TRUE])
UdjinM6
previously approved these changes
Aug 21, 2024
knst
previously approved these changes
Aug 26, 2024
There was a problem hiding this comment.
utACK 0c076fc
$ git range-diff origin/develop cb0c7eb5eaf3fb96fcaf815cedf8dec5f7d0f06d 0c076fc74082d992068a48dda5194e353d1a5202
1: 1327d74ad1 ! 1: 04d3f17da7 Merge bitcoin/bitcoin#22568: test: add addr-fetch peer connection state and timeout coverage
@@ Commit message
Code review ACK f8d8eb5fdaa93b6e5b77fd901b94927dc3a0473e
Tree-SHA512: 9a13a705d1da6b308d6dcbc6930575205e2e88bfe9f2e7cb4e0c4c40d26538430e6b02c6c772d0cee64e534777348291469a139f99afbf9d4f93f31b9e7b0818
+ Signed-off-by: Vijay <[email protected]>
## test/functional/p2p_addrfetch.py ##
@@ test/functional/p2p_addrfetch.py: ADDR.port = 18444
2: cedba33af8 ! 2: 82f7b660bc Merge bitcoin/bitcoin#21800: mempool/validation: mempool ancestor/descendant limits for packages
@@ Commit message
ACK accf3d5.
Tree-SHA512: 0d18ce4b77398fe872e0b7c2cc66d3aac2135e561b64029584339e1f4de2a6a16ebab3dd5784f376e119cbafc4d50168b28d3bd95d0b3d01158714ade2e3624d
+ Signed-off-by: Vijay <[email protected]>
## src/rpc/rawtransaction.cpp ##
@@ src/rpc/rawtransaction.cpp: static RPCHelpMan testmempoolaccept()
3: cb0c7eb5ea ! 3: 0c076fc740 (partial) Merge bitcoin/bitcoin#22707: test: refactor use of getrawmempool in functional tests for efficiency
@@ Commit message
Tested ACK 47c48b5f35b4910fcf87caa6e37407e67d879c80
Tree-SHA512: 40f553715f3d4649dc18c2738554eafaca9ea800c4b028c099217896cc1c466ff457ae814d59cf8564c782a8964d8fac3eda60c1b6ffb08bbee1439b2d34434b
+ Signed-off-by: Vijay <[email protected]>
## test/functional/mempool_compatibility.py ##
@@ test/functional/mempool_compatibility.py: class MempoolCompatibilityTest(BitcoinTestFramework):
UdjinM6
previously approved these changes
Aug 26, 2024
|
This pull request has conflicts, please rebase. |
…imeout coverage f8d8eb5 test: add addr-fetch timeout connection coverage in p2p_addrfetch.py (Jon Atack) 9321086 test: add assert_getpeerinfo method and coverage in p2p_addrfetch.py (Jon Atack) Pull request description: This patch adds additional addr-fetch peer connection state and timeout coverage as a follow-up to bitcoin#22096. ACKs for top commit: Saviour1001: Tested ACK <code>[f8d8eb5](https://github.com/bitcoin/bitcoin/pull/22568/commits/f8d8eb5fdaa93b6e5b77fd901b94927dc3a0473e)</code> mzumsande: Code review ACK f8d8eb5 Tree-SHA512: 9a13a705d1da6b308d6dcbc6930575205e2e88bfe9f2e7cb4e0c4c40d26538430e6b02c6c772d0cee64e534777348291469a139f99afbf9d4f93f31b9e7b0818 Signed-off-by: Vijay <[email protected]>
…limits for packages accf3d5 [test] mempool package ancestor/descendant limits (glozow) 2b6b26e [test] parameterizable fee for make_chain and create_child_with_parents (glozow) 313c09f [test] helper function to increase transaction weight (glozow) f8253d6 extract/rename helper functions from rpc_packages.py (glozow) 3cd663a [policy] ancestor/descendant limits for packages (glozow) c6e016a [mempool] check ancestor/descendant limits for packages (glozow) f551841 [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits (glozow) 97dd1c7 MOVEONLY: add helper function for calculating ancestors and checking limits (glozow) f95bbf5 misc package validation doc improvements (glozow) Pull request description: This PR implements a function to calculate mempool ancestors for a package and enforces ancestor/descendant limits on them as a whole. It reuses a portion of `CalculateMemPoolAncestors()`; there's also a small refactor to move the reused code into a generic helper function. Instead of calculating ancestors and descendants on every single transaction in the package and their ancestors, we use a "worst case" heuristic, treating every transaction in the package as each other's ancestor and descendant. This may overestimate everyone's counts, but is still pretty accurate in the our main package use cases, in which at least one of the transactions in the package is directly related to all the others (e.g. 1 parent + 1 child, multiple parents with 1 child, or chains). Note on Terminology: While "package" is often used to describe groups of related transactions _within_ the mempool, here, I only use package to mean the group of not-in-mempool transactions we are currently validating. #### Motivation It would be a potential DoS vector to allow submission of packages to mempool without a proper guard for mempool ancestors/descendants. In general, the purpose of mempool ancestor/descendant limits is to limit the computational complexity of dealing with families during removals and additions. We want to be able to validate multiple transactions on top of the mempool, but also avoid these scenarios: - We underestimate the ancestors/descendants during package validation and end up with extremely complex families in our mempool (potentially a DoS vector). - We expend an unreasonable amount of resources calculating everyone's ancestors and descendants during package validation. ACKs for top commit: JeremyRubin: utACK accf3d5 ariard: ACK accf3d5. Tree-SHA512: 0d18ce4b77398fe872e0b7c2cc66d3aac2135e561b64029584339e1f4de2a6a16ebab3dd5784f376e119cbafc4d50168b28d3bd95d0b3d01158714ade2e3624d Signed-off-by: Vijay <[email protected]>
… functional tests for efficiency 47c48b5 test: only use verbose for getrawmempool when necessary in functional tests (Michael Dietz) 7734971 test: use getmempoolentry instead of getrawmempool in functional tests when appropriate (Michael Dietz) 86dbd54 test: improve mempool_updatefrom efficiency by using getmempoolentry for specific txns (Michael Dietz) Pull request description: I don't think this changes the intention of the test. But it does shave ~30 seconds off the time it takes to run. From what I've seen our CI `macOS 11 native [gui] [no depends]` runs `mempool_updatefrom.py` in ~135 seconds. After this PR it should run in ~105 seconds I noticed this improvement should probably be made when testing performance/runtimes of bitcoin#22698. But I wanted to separate this out from that PR so the affects of each is decoupled Edit: The major change in this PR is improving mempool_updatefrom.py's runtime as this is a very long running test. Then made the same efficiency improvements across all the functional tests as it made since to do that here ACKs for top commit: theStack: Tested ACK 47c48b5 Tree-SHA512: 40f553715f3d4649dc18c2738554eafaca9ea800c4b028c099217896cc1c466ff457ae814d59cf8564c782a8964d8fac3eda60c1b6ffb08bbee1439b2d34434b Signed-off-by: Vijay <[email protected]>
|
Hello @PastaPastaPasta @UdjinM6 @knst , had to rebase because of merge conflict, requesting re-approval/review |
UdjinM6
approved these changes
Aug 29, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
bitcoin backports