Make bitcoin-tx replaceable value optional #29022

kashifs commented at 11:24 AM on December 7, 2023: contributor

This fixes #28638. The issue was originally raised by dooglus, who also suggested the patch found in this code. Additionally, test coverage has been added and documentation has been updated.

bitcoin-tx: Make replaceable value optional c2b836b119

DrahtBot commented at 11:24 AM on December 7, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pinheadmz, hernanmarino, instagibbs, achow101

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

fanquake renamed this:
~~Make bitcoin-tx replaceable value optional, #fixes 28638~~
Make bitcoin-tx replaceable value optional
on Dec 7, 2023

DrahtBot added the label CI failed on Dec 7, 2023

kashifs force-pushed on Dec 7, 2023

in src/bitcoin-tx.cpp:71 in 605a147b3b outdated

  65 | @@ -66,7 +66,9 @@ static void SetupBitcoinTxArgs(ArgsManager &argsman)
  66 |      argsman.AddArg("outscript=VALUE:SCRIPT[:FLAGS]", "Add raw script output to TX. "
  67 |          "Optionally add the \"W\" flag to produce a pay-to-witness-script-hash output. "
  68 |          "Optionally add the \"S\" flag to wrap the output in a pay-to-script-hash.", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
  69 | -    argsman.AddArg("replaceable(=N)", "Set RBF opt-in sequence number for input N (if not provided, opt-in all available inputs)", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);
  70 | +    argsman.AddArg("replaceable(=N)", "Sets Replace-By-Fee (RBF) opt-in sequence number for input N. "
  71 | +        "If N is not provided, the command attempts to opt-in all available inputs for RBF. "
  72 | +        "If the transaction has no inputs, no action will be taken by this command. ", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS);

pinheadmz commented at 4:50 PM on December 8, 2023:

I think you can just remove this last sentence. The command DOES take an action if no inputs are provided (you added a test for this!), and the impact of this option in that case I think is handled already by the first two sentences.

kashifs commented at 12:38 AM on December 9, 2023:

Hi @pinheadmz, Thanks for taking the time to review this. I've updated the Makefile.

For the documentation, I wanted to make it clear to the end user that if a transaction with no inputs called tx_noinputs_replaceable is created by running the following command:./bitcoin-tx -create replaceable, it should NOT be assumed that inputs later added to tx_noinputs_replaceable will default to being replaceable.

Is this already obvious? If so, I can remove the line. If not, is there other language that I should use to make this clear?

Thanks!

pinheadmz commented at 3:05 PM on December 11, 2023:

Ok I see maybe then "If the transaction has no inputs, this option is ignored"? is that correct?

kashifs commented at 6:22 PM on December 11, 2023:

Yes, that's correct. I've updated the text to read as you suggested. Thanks!

pinheadmz commented at 5:12 PM on December 8, 2023: member

concept ACK 605a147b3bf93b7e4325afa4a4df61ffb3523012

I think the test failures are because on CI we copy everything we need for testing into a new directory in the container, and the files you added need to be listed as well in Makefile.md under EXTRA_DIST.

For extra context, this is the command that copies the files from EXTRA_DIST into the ci scratch/build working directory.

You can run CI locally with docker (see ci/README.md). Just for sanity check I entered the test container to prove the files were missing:

root@3e18724095c4:/# diff /Users/matthewzipkin/Desktop/work/bitcoin/test/util/data/ /ci_container_base/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/test/util/data/
Only in /Users/matthewzipkin/Desktop/work/bitcoin/test/util/data/: txreplace1.hex
Only in /Users/matthewzipkin/Desktop/work/bitcoin/test/util/data/: txreplacenoinputs.hex
Only in /Users/matthewzipkin/Desktop/work/bitcoin/test/util/data/: txreplaceomittedn.hex
Only in /Users/matthewzipkin/Desktop/work/bitcoin/test/util/data/: txreplacesingleinput.hex

kashifs force-pushed on Dec 9, 2023

tests: Add unit tests for bitcoin-tx replaceable command 94feaf2b66

kashifs force-pushed on Dec 9, 2023

DrahtBot removed the label CI failed on Dec 9, 2023

doc: Update bitcoin-tx replaceable documentation 98afe78661

kashifs force-pushed on Dec 11, 2023

pinheadmz approved

pinheadmz commented at 6:35 PM on December 13, 2023: member

ACK 98afe7866185ed4157ffc581763e11dc02fcbae0

Great work Kashif! Built and ran all tests locally. Confirmed that some new tests fail without the patch, and additional tests were added that expand coverage for the bitcoin-tx utility with "replaceable" option. I also played with the updated utility on the command line, and confirmed that this patch closes the issue linked in the PR description. 👍

<details><summary>Show Signature</summary>

pinheadmz's public key is on keybase

</details>

hernanmarino approved