test: pass `dustrelayfee=0` option for tests using dust (instead of `acceptnonstdtxn=1`) #25535

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202207-test-replace_acceptnonstdxn_with_dustrelayfee_option changing 2 files +6 −4

theStack commented at 2:25 PM on July 3, 2022: member

By specifying the dustrelayfee=0 option instead of the more generic acceptnonstdtxn=1, we can be more specific about what part of the transaction is non-standard and can be sure that all other aspects follow the standard policy. Note that for the test feature_dbcrash.py, the UTXO creation at the start of the test has to be split up to several txs to not exceed the tx standard size limit of 100k vbytes

https://github.com/bitcoin/bitcoin/blob/4129c1375430dbfe8dd414868c43fceb3d091fc3/src/policy/policy.h#L26-L27
fanquake added the label Tests on Jul 3, 2022

test: pass `dustrelayfee=0` option for tests using dust (instead of `acceptnonstdtxn=1`)

By specifying the `dustrelayfee=0` option instead of the more
generic `acceptnonstdtxn=1`, we can be more specific about what
part of the transaction is non-standard and can be sure that all
other aspects follow the standard policy.

1770be72d5

theStack force-pushed on Jul 3, 2022

in test/functional/feature_dbcrash.py:216 in 1770be72d5

 210 | @@ -211,7 +211,9 @@ def run_test(self):
 211 |          self.crashed_on_restart = 0      # Track count of crashes during recovery
 212 |  
 213 |          # Start by creating a lot of utxos on node3
 214 | -        utxo_list = self.wallet.send_self_transfer_multi(from_node=self.nodes[3], num_outputs=5000)['new_utxos']
 215 | +        utxo_list = []
 216 | +        for _ in range(5):
 217 | +            utxo_list.extend(self.wallet.send_self_transfer_multi(from_node=self.nodes[3], num_outputs=1000)['new_utxos'])

MarcoFalke commented at 5:23 AM on July 4, 2022:

why is this needed?

theStack commented at 6:26 AM on July 4, 2022:

To not exceed MAX_STANDARD_TX_WEIGHT, see PR description.

MarcoFalke commented at 6:28 AM on July 4, 2022: member

review ACK 1770be72d5eb47cae565d4ac86de5bc16f94fdd6
MarcoFalke merged this on Jul 4, 2022
MarcoFalke closed this on Jul 4, 2022
theStack deleted the branch on Jul 4, 2022
sidhujag referenced this in commit 5920ac33c7 on Jul 4, 2022
luke-jr commented at 1:41 AM on July 6, 2022: member

and can be sure that all other aspects follow the standard policy.

But that's not what is being tested here. This (and the last one like it) are going in the wrong direction. We should be disabling unrelated checks, and only testing the one we're trying to test.
luke-jr commented at 1:42 AM on July 6, 2022: member

And @MarcoFalke, why are you again rush-merging underreviewed PRs?
MarcoFalke commented at 6:26 AM on July 6, 2022: member

We should be disabling unrelated checks, and only testing the one we're trying to test.

I am pretty sure this exact topic was discussed previously (years ago), so I am currently not interested in re-hashing it.

If this was something we were doing, it should be disabled globally for all tests, and individual tests that need it could enable it. Though -- obviously -- we are not doing that.

Pull requests with suggestions or reversion, or issues to discuss on a case-by-case basis are welcome.
DrahtBot locked this on Jul 6, 2023

Contributors

Labels