added bumpfee.py script for bumping fees for rbf transactions

[takinbo]

Following the merging of JoinMarket-Org/joinmarket-clientserver#921, this PR adds the bumpfee.py script to enable fee bumping of replaceable transactions in JoinMarket.

[AdamISZ]

Thanks for the PR, useful stuff.

As well as the notes above I have another two general comments:

• We'll want to have a test case of bumping using these functions, as well as the one we have currently from #921, using these new functions.
• Consider whether it'll be easy for Qt to implement the same function (I think it probably will but I didn't analyze it yet).

Actually the above two points together imply one thing in common: move the functional steps (i.e. everything outside __main__ in this script, to a module in jmclient that can be accessed. You can make a new jmclient/bumpfee.py or whatever name you think works.

[AdamISZ]

@takinbo are you not able to work on this one any more?

(Tbh it's fine either way; I think this would be a nice addition to the wallet, but also there is no specific timeframe on it. )

Also if @takinbo doesn't have time, and anyone else would like to address my comments above, they could just open another PR I guess.

[takinbo]

I would find some time over the holidays to finish it up but I'll be happy to review a submission if anyone has time for it right now.

[AdamISZ]

@takinbo this kind of mess up happens sometimes :) Sometimes the easiest thing to do is just open a new PR.

[takinbo]

:( Yes. I'll open a new one.

[AdamISZ]

:( Yes. I'll open a new one.

I guess you probably already know, but just in case it helps: git cherry-pick <hash> can be a useful way to put already-existing code patches on top of somewhere new. Of course if there are conflicts though, there's still work to do ( git mergetool or whatever).

[AdamISZ]

And also I don't know what you previously did here, but it should work (always, I think?) if you do git rebase master and then resolve conflicts commit by commit using mergetool and then at the end force-push git push -f to your branch. But I hesitate to comment too much as I only know a certain way of doing things in git, I am not an expert.

[takinbo]

Thanks for the suggestions. You're right that my branch was old, I must have messed something up when I did the last rebase. I'll resolve this and update.

[AdamISZ]

I'm glad you figured out the git issues, I'm ready (kinda, depending on time) to review this more. But I guess I should wait till you address the existing comments before going further.

[AdamISZ]

Looking good. Hopefully someone (includes me) can test it out shortly. The added test functions should be a big help, thanks.

[kristapsk]

Had forgotten about this. Tests pass. Will try to test on signet soon.

[kristapsk]

tACK. Merging this. Minor issues can be solved on top with additional commits, this has been staying unmerged for too long already.

[kristapsk]

@takinbo Could you squash commits?

[AdamISZ]

I'm in the process of taking another look at it now, but one thing is: let's double check whether the significant changes to tx size/ fee size calcs in #1421 affects this.

[takinbo]

I'm attending the Advancing Bitcoin conference this week. I'll be happy to take another look re: #1421 and make any necessary changes next week.

[AdamISZ]

A report on testing:

git rebase master was successful.

I tried to go through a fairly vanilla example of replacement on regtest. It seemed to work, but, in detail:

1. Edit test/ygrunner.py to make the block creation interval ~ infinite ( jm_single().bc_interface.tick_forward_chain_interval = 1000).
2. Run the above in the usual way (see testing docs), so the blockchain is up and you have wallets with coins.
3. When you need to mine a block, use bitcoin-cli -regtest -conf=/home/waxwing/bitcoin.conf generatetoaddress 1 2N2JD6wb56AfK4tfmM6PwdVmoYk2dCKf4Br or anything similar. This lets us create the tx, examine it, bump it and then mine it.
4. Noting the hex seed for the taker test wallet from the output of ygrunner.py, broadcast a spend, from its m0 or m1 to, e.g. its m4 (so you can easily see the destination whenever you run wallet-tool.py): python sendpayment.py --datadir=. --rbf -m1 685ac0856ed650568aec08571fe464ff 0.22 bcrt1q3hwk6jgapw8z2jss3z8hc4rvlvazpdvrzhhtvh
5. Run wallet-tool.py here and check everything is as expected, and the destination coins are PENDING (see Step 1).
6. Can also check the output of the showutxos method here, and after bump, and look at txids of utxos.
7. Note the index of the change address. Here it was 1. Run bump script: python bumpfee.py --datadir=. -o 1 -f 16000 685ac0856ed650568aec08571fe464ff 6030b09b59a555bb174ffa0f2496b5b21cbd3c1d8a2358cb8124a6144ab5b7cc. I used 16 sats/vb here (see note at end).
8. Do steps 5 and 6 again and note that the txid has changed, and also the output amount has changed.
9. Mine a block then repeat 5 and 6 again and check everything is as expected.

I think this is viable right now, I will note one aspect of the experience that was a little unclear: our human_readable_transaction code is not outputting the fee that was paid (though note, it is shown clearly in the output of sendpayment.py. I think this could be changed in a later patch.

I think this is pretty much ready (though I might well have missed something!), but @takinbo if you could just (1) squash these commits and then (2) rebase on master, and check that it looks good to you? Then we can have a final read and merge it.

[AdamISZ]

A minor thing, but tested that trying to run bumpfee.py on a txid where we didn't use --rbf in the options to sendpayment, "failed successfully" in that the script just reports "Transaction not replaceable".

[AdamISZ]

Another minor test, but can also: use --psbt as argument to bumpfee.py, was able to retrieve that PSBT and broadcast it separately using Core, and verified as above that the replacement happened correctly.

[kristapsk]

I did my testing on signet. But signet does not have right economic incentives, at the end first transaction was included in the block, not the replacement. But Core did abandon the first tx and showed replacement in tx history (until confirmation).

[kristapsk]

After this is merged, I plan to work on also adding CPFP fee bumping support here.

[AdamISZ]

I did my testing on signet. But signet does not have right economic incentives, at the end first transaction was included in the block, not the replacement. But Core did abandon the first tx and showed replacement in tx history (until confirmation).

Interesting report.

I'm trying to reproduce this issue. I have done a replacement, that is currently on mempool.space, showing as a successful replacement:

https://mempool.space/signet/tx/bc6b179170c5cc804d53e941bececd1e4a20e3e8e0f9064afb59ebc9a5a45d62

edit: and the replacement indeed confirmed.

[AdamISZ]

Did you make sure your fee was low? I used tx_fees_factor=0 to ensure that my 1050 sats/kvb was respected. Then used 16000 sats/kvb for the replacement.

[kristapsk]

This was transaction included in block - https://explorer.bc-2.jp/tx/dff7d81de0cc49ec313b3651973752ad1d9afbb95b70d15a61ad89e55c592db4. Replacement, which wasn't "mined", sent 0.00147212 BTC to tb1qdh3sruh2z2mrrrjvw596ghcyc23ln0x3k4hg29, so paid 1164 sats more in fees.

$ bitcoin-cli -signet decoderawtransaction 02000000000101c41c1039525c0381215c5eee0362eb29f921038819dc1f9de058d6420d1ee38f0000000000fdffffff020c3f0200000000001600146de301f2ea12b6318e4c750ba45f04c2a3f9bcd1a0860100000000001600145fa15656e63fea2e582956c408270ffe3522a5d00247304402200bc2ea2b7173715fa3bd6f1e9786cd11cb9610da86d5b0afeda29e88d048b2dc02201db3cab4a9b48f4516488dd970b9a19521684fa90efb776b3c06957cc71687a7012103aa2d85b003e804db2e7a44e6ed61bcbc7bad538ff188feac513fd9c4d26c3c8a96040200
{
  "txid": "b591045af7fd7e7047c905cc9032764fcd6dc3f51ac12cb107ab3095014727c8",
  "hash": "b64f97a01911e335f4a2bab3fbd25e9170b06b1dc816b74b4b734ef895b5f74a",
  "version": 2,
  "size": 222,
  "vsize": 141,
  "weight": 561,
  "locktime": 132246,
  "vin": [
    {
      "txid": "8fe31e0d42d658e09d1fdc19880321f929eb6203ee5e5c2181035c5239101cc4",
      "vout": 0,
      "scriptSig": {
        "asm": "",
        "hex": ""
      },
      "txinwitness": [
        "304402200bc2ea2b7173715fa3bd6f1e9786cd11cb9610da86d5b0afeda29e88d048b2dc02201db3cab4a9b48f4516488dd970b9a19521684fa90efb776b3c06957cc71687a701",
        "03aa2d85b003e804db2e7a44e6ed61bcbc7bad538ff188feac513fd9c4d26c3c8a"
      ],
      "sequence": 4294967293
    }
  ],
  "vout": [
    {
      "value": 0.00147212,
      "n": 0,
      "scriptPubKey": {
        "asm": "0 6de301f2ea12b6318e4c750ba45f04c2a3f9bcd1",
        "hex": "00146de301f2ea12b6318e4c750ba45f04c2a3f9bcd1",
        "address": "tb1qdh3sruh2z2mrrrjvw596ghcyc23ln0x3k4hg29",
        "type": "witness_v0_keyhash"
      }
    },
    {
      "value": 0.00100000,
      "n": 1,
      "scriptPubKey": {
        "asm": "0 5fa15656e63fea2e582956c408270ffe3522a5d0",
        "hex": "00145fa15656e63fea2e582956c408270ffe3522a5d0",
        "address": "tb1qt7s4v4hx8l4zukpf2mzqsfc0lc6j9fwsswk7vd",
        "type": "witness_v0_keyhash"
      }
    }
  ]
}

[AdamISZ]

It does look like it should have replaced, because the weight was ~500 units so an extra 1162 sats should have been more than enough. But what about time windows here? Are you sure the first one didn't simply confirm very quickly, before the replacement was seen by the signer/miner? It's difficult to know for sure since we don't have perfect synchrony.

[takinbo]

Thanks for the extensive tests and review. I added a sanity check for the supplied transaction id and will fail gracefully if the transaction id does not belong to the wallet; before this, it would cause the script to crash.

[kristapsk]

Are you sure the first one didn't simply confirm very quickly, before the replacement was seen by the signer/miner? It's difficult to know for sure since we don't have perfect synchrony.

Could be the reason. Not sure how in sync are my local clock with others involved, but it looks that mined block was less than 60 seconds after I broadcast the replacement.

[AdamISZ]

tACK - pretty sure everything has been addressed and this is functional.

Todos perhaps: add this functionality to Qt application. Change our human readable transaction output to clearly show the fee paid in sats (although some sophistication is required anyway, of users, to understand what the rules are about bumping). Related: add something to USAGE.md about this function.

Merging.