test: Add test for sendall min-fee setting #26622

aureleoules commented at 7:34 PM on December 1, 2022: member

While experimenting with mutation testing it appeared that the minimum fee-rate check was not tested for the sendall RPC.

https://bcm-ui.aureleoules.com/mutations/3581479318544ea6b97f788cec6e6ef1

DrahtBot commented at 7:34 PM on December 1, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, 0xB10C, ishaanam

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

maflcko renamed this:
~~test(sendall): Add test for min-fee setting~~
test: Add test for sendall min-fee setting
on Dec 1, 2022

DrahtBot added the label Tests on Dec 1, 2022

stickies-v commented at 11:37 PM on December 1, 2022: contributor

Concept ACK, but any reason not to put this in wallet_sendall.py?

aureleoules force-pushed on Dec 2, 2022

aureleoules commented at 10:49 AM on December 2, 2022: member

Concept ACK, but any reason not to put this in wallet_sendall.py?

You're right it makes more sense. Updated.

in test/functional/wallet_sendall.py:333 in 705a03cdc7 outdated

 329 | @@ -325,6 +330,7 @@ def sendall_fails_with_transaction_too_large(self):
 330 |                  self.wallet.sendall,
 331 |                  recipients=[self.remainder_target])
 332 |  
 333 | +

stickies-v commented at 10:59 AM on December 2, 2022:

nit: this newline is not in line with PEP8

aureleoules commented at 11:59 AM on December 2, 2022:

Whoops missed this thanks

in test/functional/wallet_sendall.py:386 in 705a03cdc7 outdated

 384 | @@ -379,6 +385,9 @@ def run_test(self):
 385 |          # Sendall succeeds with watchonly wallets spending specific UTXOs
 386 |          self.sendall_watchonly_specific_inputs()
 387 |  
 388 | +        # Sendall fails when fee rate is lower than minimum
 389 | +        self.sendall_fails_on_low_fee()

stickies-v commented at 11:00 AM on December 2, 2022:

nit: would be more logical to put this right before/after the high fees test?

in test/functional/wallet_sendall.py:316 in 705a03cdc7 outdated

 310 | @@ -311,6 +311,11 @@ def sendall_watchonly_specific_inputs(self):
 311 |          assert_equal(decoded["tx"]["vin"][0]["vout"], utxo["vout"])
 312 |          assert_equal(decoded["tx"]["vout"][0]["scriptPubKey"]["address"], self.remainder_target)
 313 |  
 314 | +    @cleanup
 315 | +    def sendall_fails_on_low_fee(self):
 316 | +        assert_raises_rpc_error(-8, "Fee rate (0.999 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)",

stickies-v commented at 11:00 AM on December 2, 2022:

every other test starts with a self.log.info, probably best to be consistent?

stickies-v commented at 11:01 AM on December 2, 2022: contributor

Approach ACK 705a03cdc7a6e0feee24a0522162c53cc9b9ff00

aureleoules force-pushed on Dec 2, 2022

aureleoules commented at 12:00 PM on December 2, 2022: member

Updated per feedback, thanks @stickies-v.

in test/functional/wallet_sendall.py:289 in 1bfa0de5f4 outdated

 283 | @@ -284,6 +284,12 @@ def sendall_fails_on_high_fee(self):
 284 |                  recipients=[self.remainder_target],
 285 |                  fee_rate=100000)
 286 |  
 287 | +    @cleanup
 288 | +    def sendall_fails_on_low_fee(self):
 289 | +        self.log.info("Test sendall fails if the transaction fee is too low")

stickies-v commented at 12:13 PM on December 2, 2022:

nit: would prefer to be precise

        self.log.info("Test sendall fails if the transaction fee is lower than the minimum fee rate setting")

aureleoules commented at 12:30 PM on December 2, 2022:

Done

in test/functional/wallet_sendall.py:291 in 1bfa0de5f4 outdated

 283 | @@ -284,6 +284,12 @@ def sendall_fails_on_high_fee(self):
 284 |                  recipients=[self.remainder_target],
 285 |                  fee_rate=100000)
 286 |  
 287 | +    @cleanup
 288 | +    def sendall_fails_on_low_fee(self):
 289 | +        self.log.info("Test sendall fails if the transaction fee is too low")
 290 | +        assert_raises_rpc_error(-8, "Fee rate (0.999 sat/vB) is lower than the minimum fee rate setting (1.000 sat/vB)",
 291 | +        self.wallet.sendall, recipients=[self.recipient], options={"fee_rate": 0.999})

stickies-v commented at 12:21 PM on December 2, 2022:

nit: fee_rate is also defined as a regular argument, so no need to use options

        self.wallet.sendall, recipients=[self.recipient], fee_rate=0.999)

aureleoules commented at 12:30 PM on December 2, 2022:

Done

stickies-v approved

stickies-v commented at 12:26 PM on December 2, 2022: contributor

ACK 1bfa0de5f

test: Add sendall test for min-fee setting cb44c5923a

aureleoules force-pushed on Dec 2, 2022

stickies-v approved

stickies-v commented at 12:33 PM on December 2, 2022: contributor

re-ACK cb44c59

0xB10C commented at 5:36 PM on December 2, 2022: contributor

ACK cb44c5923a7c0ba15400d2b420faedd39cdce128

Applied the diff from your mutation test and checked that the test now fails and would kill the mutation.

Maybe a feature to consider for your mutation testing: Pushing a failed mutation diff to a branch somewhere. Allows to quickly cherry-pick the fix on top and test that it's fixed.

aureleoules commented at 6:27 PM on December 2, 2022: member

Maybe a feature to consider for your mutation testing: Pushing a failed mutation diff to a branch somewhere. Allows to quickly cherry-pick the fix on top and test that it's fixed.

Thanks, will look into it.

ishaanam commented at 9:55 PM on December 2, 2022: contributor

ACK cb44c5923a7c0ba15400d2b420faedd39cdce128

maflcko merged this on Dec 3, 2022

maflcko closed this on Dec 3, 2022

sidhujag referenced this in commit 5ec826c869 on Dec 4, 2022

aureleoules deleted the branch on Jan 12, 2023

bitcoin locked this on Jan 12, 2024