On the main network, nonstandard transactions are hardly relayed, so it seems odd that one of our policy test requires a policy setting opposite of the norm.
Surely it is also important to test that nonstandard transactions can be replaced. However, rbf code should not care about the standardness at all. Moreover, I think testing nonstandardness rbf is of lower priority than testing the stuff that actually happens in production.
Strong Concept ACK
Previously it was only possible to set the same sequence in all inputs
This makes individual bytes of the scriptPubKey mutable, previously it
could only be re-assigned as a whole.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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.
121 | 122 | # Should fail because we haven't changed the fee 123 | - tx1b = deepcopy(tx_template) 124 | - tx1b.vout = [CTxOut(1 * COIN, DUMMY_2_P2WPKH_SCRIPT)] 125 | - tx1b_hex = tx1b.serialize().hex() 126 | + tx.vout[0].scriptPubKey[-1] -= 1
This would fail if the scriptPubKey ends with a zero byte, e.g.:
>>> spk = bytearray(b'\xbe\xef\x00')
>>> spk[-1] -= 1
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: byte must be in range(0, 256)
Modifying it with a bit-flip is probably slightly preffered:
>>> spk[-1] ^= 1
>>> spk
bytearray(b'\xbe\xef\x01')
tx.vout[0].scriptPubKey[-1] ^= 1
Thx.
The test should be deterministically pass even with -=1, but I've switched to ^=1 as it is cleaner.
Concept ACK, will review early next week
245 | - dbl_tx.vout = [CTxOut(initial_nValue - 2 * fee * n, DUMMY_P2WPKH_SCRIPT)] 246 | - dbl_tx_hex = dbl_tx.serialize().hex() 247 | + dbl_tx_hex = self.wallet.create_self_transfer( 248 | + utxo_to_spend=tx0_outpoint, 249 | + sequence=0, 250 | + fee=2 * (Decimal(fee) / COIN) * n + Decimal("0.1"),
Is the last part really needed?
fee=2 * (Decimal(fee) / COIN) * n,
Good catch, fixed!
Code-review ACK faf9f0005ca1ebcea461f66191efdc78af744660
Very nice to see MiniWallet unfolding its full power! 🦾 the code is much more easier to reason about, without the need of manually crafting transactions. Unrelated to this PR, but I think the current mixture of different units for both absolute values/fees and fee-rates is a bit messy -- maybe we should aspire to only use sats and sats/vbyte at some point, which would get rid of many unneeded * COIN and / COIN operations.
re-ACK fad690ba0a62dfd97ef22711b2344909fcd1fb73
The current CI failures seem to be unrelated to this PR.
(ci rerun after outage, now green)
231 | @@ -232,12 +232,14 @@ def create_self_transfer_multi( 232 | *, 233 | utxos_to_spend: Optional[List[dict]] = None, 234 | num_outputs=1, 235 | + amount_per_output=0,
https://github.com/bitcoin/bitcoin/pull/25522/commits/fac3800d2c962420303ab5c61b2f02f35b9f693a
(only if you retouch) might be clearer to make this None
ACK fad690ba0a62dfd97ef22711b2344909fcd1fb73 (jamesob/ackr/25522.1.MarcoFalke.test_remove_acceptnonstd)
Good change - significantly simplifies the RBF tests and allows us to avoid uncharacteristic use of nonstandard transactions.