test: Remove -acceptnonstdtxn=1 from feature_rbf.py #25522
Conversation
ddc5efe to
065aeea
Compare
|
Strong Concept ACK |
…sfer_multi 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.
065aeea to
fa034ad
Compare
|
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. |
fa034ad to
faf9f00
Compare
|
Concept ACK, will review early next week |
There was a problem hiding this comment.
Code-review ACK faf9f00
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.
faf9f00 to
fad690b
Compare
|
(ci rerun after outage, now green) |
There was a problem hiding this comment.
ACK fad690b (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.
| @@ -232,12 +232,14 @@ def create_self_transfer_multi( | |||
| *, | |||
| utxos_to_spend: Optional[List[dict]] = None, | |||
| num_outputs=1, | |||
| amount_per_output=0, | |||
There was a problem hiding this comment.
(only if you retouch) might be clearer to make this None
…g_transactions` helper 6cbe65c test: refactor: pass absolute fee in `create_lots_of_big_transactions` helper (Sebastian Falbesoner) Pull request description: Recently merged PR #25522 (commit 2222842) enabled specifying an absolute fee for MiniWallet's `create_self_transfer` method. We can use that in the `create_lots_of_big_transactions` helper to avoid deducting the fee manually (with prior conversion from BTC to Satoshis). This helper is used (directly or indirectly) in the tests `feature_maxuploadtarget.py`, `mempool_limit.py`, `mining_prioritisetransaction.py`. ACKs for top commit: MarcoFalke: cr ACK 6cbe65c Tree-SHA512: 63d66939ae36722a2dc787cbd8f1f995de6232139c2169a3d25525f43c7aaacf646d86b4095a8078f26db18e916778c8097acb19ef17ab0f58382b8bb718d60b
…s_of_big_transactions` helper 6cbe65c test: refactor: pass absolute fee in `create_lots_of_big_transactions` helper (Sebastian Falbesoner) Pull request description: Recently merged PR bitcoin#25522 (commit 2222842) enabled specifying an absolute fee for MiniWallet's `create_self_transfer` method. We can use that in the `create_lots_of_big_transactions` helper to avoid deducting the fee manually (with prior conversion from BTC to Satoshis). This helper is used (directly or indirectly) in the tests `feature_maxuploadtarget.py`, `mempool_limit.py`, `mining_prioritisetransaction.py`. ACKs for top commit: MarcoFalke: cr ACK 6cbe65c Tree-SHA512: 63d66939ae36722a2dc787cbd8f1f995de6232139c2169a3d25525f43c7aaacf646d86b4095a8078f26db18e916778c8097acb19ef17ab0f58382b8bb718d60b
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.