Improve mempool_package_limits.py #22901
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
glozow
left a comment
glozow
left a comment
There was a problem hiding this comment.
Concept ACK, thanks for working on this
Linking the suggestions in the PR description and/or describing the changes within the commit messages would help reviewers understand what the improvements are. Also, you seem to have a merge commit that should be squashed?
| assert signedtx["complete"] | ||
| tx = tx_from_hex(signedtx["hex"]) | ||
| return (tx, signedtx["hex"], my_value, tx.vout[0].scriptPubKey.hex()) | ||
| return tx, signedtx["hex"], my_value, tx.vout[0].scriptPubKey.hex() |
There was a problem hiding this comment.
I think #21800 (comment) suggested changing this return value to a named tuple instead?
| """Pad a transaction with extra outputs until it reaches a target weight (or higher). | ||
| returns CTransaction object | ||
| """ | ||
| tx_heavy = deepcopy(tx) |
There was a problem hiding this comment.
Can also remove the import as well, iirc
|
|
||
| def make_chain(node, address, privkeys, parent_txid, parent_value, n=0, parent_locking_script=None, fee=DEFAULT_FEE): | ||
| """Build a transaction that spends parent_txid.vout[n] and produces one output with | ||
| def make_chain(node, address, privkeys, parent_txid, parent_value, parent_locking_script=None, fee=DEFAULT_FEE): |
There was a problem hiding this comment.
https://github.com/bitcoin/bitcoin/pull/21800/files#r685091478 also suggested renaming this function
I'd suggest something like create_child ?
| node = self.nodes[0] | ||
| first_coin = self.coins.pop() | ||
| (chain_hex, chain_txns) = create_raw_chain(node, first_coin, self.address, self.privkeys) | ||
| chain_hex, chain_txns = create_raw_chain(node, first_coin, self.address, self.privkeys) |
There was a problem hiding this comment.
You need to add the chain length here now that the default has been removed
|
Concept ACK. Will review once @glozow's comments have been addressed and the CI failures are fixed. |
|
@naiza2000 still working on this? |
Yes, I was out of town for the last few days, I have started working on it again. |
|
Needs rebase and squash? |
|
Can be closed? |
Seems so. I can open a PR to clean up all the followups from #21800 |
6 participants
Follow-up to #21800 adding changes suggested in #21800 (review)