tests: Fix wallet_resendwallettransactions.py intermittent failure by using manual bumps instead of bumpfee #28540

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:resend-test-feerate-exp-fix changing 1 files +23 −3

achow101 commented at 9:49 PM on September 26, 2023: member

Bumpfee will try to increase the entire package to the target feerate, which causes repeated bumpfees to quickly shoot up in fees, causing intermittent failures when the fee is too large. We don't care about this property, just that the child is continuously replaced until we observe it's position in mapWallet is before its parent. Instead of using bumpfee, we can create raw transactions which have only pay (just above) the additional incremental relay fee, thus avoiding this problem.

Fixes #28491
DrahtBot commented at 9:49 PM on September 26, 2023: 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 mzumsande, pablomartin4btc, kevkevinpal, MarcoFalke

Stale ACK furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Tests on Sep 26, 2023
achow101 force-pushed on Sep 26, 2023
DrahtBot added the label CI failed on Sep 26, 2023
DrahtBot removed the label CI failed on Sep 27, 2023
maflcko commented at 6:20 AM on September 27, 2023: member

Nice, lgtm ACK 42056e6a2944b3f61aed376b3317551ca1241971
furszy commented at 11:09 AM on September 27, 2023: member

ACK 42056e6a
in test/functional/wallet_resendwallettransactions.py:106 in 42056e6a29 outdated
```
 101 |              if txids == [child_txid, txid]:
 102 |                  break
 103 | -            bumped = node.bumpfee(child_txid)
 104 | +            # Manually bump the tx
 105 | +            # The inputs and the output address stay the same, just changing the amount for the new fee
 106 | +            child_output_value -= additional_child_fee
```
hebasto commented at 12:05 PM on September 27, 2023:

Can the child_output_value potentially become negative?

maflcko commented at 12:11 PM on September 27, 2023:

Maybe? But this seems unrelated to this pull request, as it should only restore the old behavior before #28491 (comment)

achow101 commented at 3:07 PM on September 27, 2023:

No, it would reach the max tx fee constraint first. The output being spent is 1 BTC, and the amount being reduced is ~141 sats each iteration. You'd have to be extremely unlucky for it to even get to that point.
maflcko added this to the milestone 26.0 on Sep 27, 2023
achow101 force-pushed on Sep 27, 2023

Type	Reviewers
ACK	mzumsande, pablomartin4btc, kevkevinpal, MarcoFalke
Stale ACK	furszy

tests: Use manual bumps instead of bumpfee for resendwallettransactions

Bumpfee will try to increase the entire package to the target feerate,
which causes repeated bumpfees to quickly shoot up in fees, causing
intermittent failures when the fee is too large. We don't care about
this property, just that the child is continuously replaced until we
observe it's position in mapWallet is before its parent. Instead of
using bumpfee, we can create raw transactions which have only pay the
additional incremental relay fee, thus avoiding this problem.

b5a962564e

achow101 force-pushed on Sep 27, 2023
DrahtBot added the label CI failed on Sep 27, 2023
achow101 commented at 3:40 PM on September 27, 2023: member

@mzumsande noticed that the loop would fail a lot earlier than expected due to the variability of ecdsa signatures causing the feerate to be a bit higher than expected in one iteration, and then falling below that in the next iteration. I've added a fix for this where we just ignore that error and move on to the next iteration.

This appears to eventually hit the max feerate after ~9000 iterations, which I think is high enough that we shouldn't hit this in practice.
mzumsande commented at 5:41 PM on September 27, 2023: contributor

Code review ACK b5a962564eb15075e4e2a7bc0c235a56fa998ac3
DrahtBot requested review from furszy on Sep 27, 2023
DrahtBot requested review from maflcko on Sep 27, 2023
pablomartin4btc commented at 5:45 PM on September 27, 2023: member

I got the intermittent failure in some PRs.

ACK b5a962564eb15075e4e2a7bc0c235a56fa998ac3 -> adding the try_rpc to avoid (skip) any possible failure around the manual bump fee (if we ever reach it as explained) makes a lot of sense as the spirit of the test is the tx (child before parent) sort in the mapWallet (as also explained).
DrahtBot removed the label CI failed on Sep 27, 2023
kevkevinpal commented at 4:08 AM on September 28, 2023: contributor

ACK b5a9625
in test/functional/wallet_resendwallettransactions.py:114 in b5a962564e
```
 111 | +            bumped_txid = node.decoderawtransaction(bumped["hex"])["txid"]
 112 | +            # Sometimes we will get a signature that is a little bit shorter than we expect which causes the
 113 | +            # feerate to be a bit higher, then the followup to be a bit lower. This results in a replacement
 114 | +            # that can't be broadcast. We can just skip that and keep grinding.
 115 | +            if try_rpc(-26, "insufficient fee, rejecting replacement", node.sendrawtransaction, bumped["hex"]):
 116 | +                continue
```
maflcko commented at 6:37 AM on September 28, 2023:

nit: Could double (or increase) the hard-coded feerate to cover this case and simplify the logic. Halving (or reducing) the maximum possible iterations should be fine.

achow101 commented at 3:09 PM on September 28, 2023:

Increasing the feerate doesn't fix this problem. It's because the size is smaller in one replacement which makes its actual feerate a bit higher, but in the next replacement, the size is normal/larger and therefore its new feerate is less than the previous. This happens regardless of the feerate.

maflcko commented at 5:52 PM on September 28, 2023:

therefore its new feerate is less than the previous.

Even if the tx size is larger by one byte, the absolute fee is increased by additional_child_fee, so the feerate should always increase if additional_child_fee is picked sufficiently large. (For example, I'd think doubling additional_child_fee should be enough, but maybe I am misunderstanding something)

achow101 commented at 5:57 PM on September 28, 2023:

It doesn't work, you can just try it yourself. Remove the break condition and the continue in this line, and see what happens.
maflcko approved
maflcko commented at 6:37 AM on September 28, 2023: member

lgtm ACK b5a962564eb15075e4e2a7bc0c235a56fa998ac3
achow101 merged this on Sep 28, 2023
achow101 closed this on Sep 28, 2023
jonesk7734 commented at 4:36 PM on September 28, 2023: none

Ok
Frank-GER referenced this in commit 2d2d0c1e13 on Oct 5, 2023
bitcoin locked this on Sep 27, 2024