rpc: Support v3 raw transactions creation #31936

pull Bue-von-hon wants to merge 4 commits into bitcoin:master from Bue-von-hon:create-v3-rawtransaction changing 8 files +29 −9
  1. Bue-von-hon commented at 7:30 am on February 22, 2025: none

    Added support for creating v3 raw transaction:

    • Overloaded to include additional parameter

    this resolves #31348

  2. DrahtBot commented at 7:30 am on February 22, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31936.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK adamandrews1
    Concept ACK glozow, 1440000bytes
    Approach NACK achow101

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28201 (Silent Payments: sending by josibake)
    • #21283 (Implement BIP 370 PSBTv2 by achow101)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label RPC/REST/ZMQ on Feb 22, 2025
  4. Bue-von-hon commented at 7:33 am on February 22, 2025: none
  5. DrahtBot added the label CI failed on Feb 22, 2025
  6. maflcko commented at 9:45 am on February 22, 2025: member
    You’ll have to add tests for any new feature and make sure the tests pass
  7. in src/rpc/rawtransaction_util.cpp:160 in 8a4348d2f1 outdated
    153@@ -154,6 +154,13 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
    154         rawTx.nLockTime = nLockTime;
    155     }
    156 
    157+    if (!version.isNull()) {
    158+        int64_t nVersion = version.getInt<int64_t>();
    159+        if (nVersion < 0 || nVersion > TRANSACTION_VERSION_MAX)
    160+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, version out of range");
    


    glozow commented at 4:02 pm on February 24, 2025:
    Perhaps include what the min and max are.

    dongwook-chan commented at 1:34 pm on March 9, 2025:
  8. in src/script/script.h:67 in 8a4348d2f1 outdated
    62@@ -63,6 +63,9 @@ static constexpr int64_t VALIDATION_WEIGHT_PER_SIGOP_PASSED{50};
    63 // How much weight budget is added to the witness size (Tapscript only, see BIP 342).
    64 static constexpr int64_t VALIDATION_WEIGHT_OFFSET{50};
    65 
    66+// Maximum transaction version
    67+static const int32_t TRANSACTION_VERSION_MAX = 3;
    


    glozow commented at 4:02 pm on February 24, 2025:
    Why aren’t we using TX_MAX_STANDARD_VERSION everywhere?

    dongwook-chan commented at 1:36 pm on March 9, 2025:
    @glozow Are you implying that we should use the variable instead of constant values in the test code?

    glozow commented at 2:48 pm on March 31, 2025:
    Yes, this seems to be redundant with TX_MAX_STANDARD_VERSION. It also appears to be unused, did you maybe forget to delete it?

    Bue-von-hon commented at 8:42 am on April 5, 2025:
    removed it.
  9. in src/rpc/rawtransaction_util.cpp:158 in 8a4348d2f1 outdated
    153@@ -154,6 +154,13 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
    154         rawTx.nLockTime = nLockTime;
    155     }
    156 
    157+    if (!version.isNull()) {
    158+        int64_t nVersion = version.getInt<int64_t>();
    


    glozow commented at 4:07 pm on February 24, 2025:
    Why int64_t? version has type uint32_t.

    dongwook-chan commented at 5:39 am on March 1, 2025:
    @glozow Thanks for the feedback. int64_t was my bad. However version field of raw transaction seems to be int32_t type according to bitcoint developer doc.. The doc explicitly states that the field is signed. Should I used unsigned instead?

    adamandrews1 commented at 10:41 pm on March 6, 2025:
    it’s weird that it’s signed, but I think we should use the same type as the docs

    rkrux commented at 12:16 pm on March 13, 2025:

    rkrux commented at 11:18 am on March 14, 2025:
    These docs are indeed outdated, last update was 4 years back: https://github.com/bitcoin-dot-org/developer.bitcoin.org. Only the code in this repo should be treated as source of truth.

    Bue-von-hon commented at 8:42 am on April 5, 2025:
    replaced it.
  10. glozow commented at 4:11 pm on February 24, 2025: member
    Concept ACK. Thanks for your PR, but it needs to pass CI and new features need tests to be considered (please read CONTRIBUTING.md). Perhaps a functional test in rpc_rawtransaction.py.
  11. in src/rpc/client.cpp:125 in 8a4348d2f1 outdated
    121@@ -122,6 +122,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
    122     { "createrawtransaction", 1, "outputs" },
    123     { "createrawtransaction", 2, "locktime" },
    124     { "createrawtransaction", 3, "replaceable" },
    125+    { "createrawtransaction", 4, "version" },
    


    luke-jr commented at 0:28 am on February 26, 2025:
    Probably best not to have this be a positional parameter.

    dongwook-chan commented at 7:15 am on March 1, 2025:
    @luke-jr We had to add it because without it, RPC method vaildation does not pass.

    luke-jr commented at 3:01 pm on March 1, 2025:
    No, the issue isn’t this one line of code, but the overall addition of the parameter

    adamandrews1 commented at 10:40 pm on March 6, 2025:
    I agree think it’s a bit too important for a 4th positional arg

    dongwook-chan commented at 1:38 pm on March 9, 2025:
    @luke-jr @adamandrews1 So should I remove the line? If I should, could you advise which code should be updated to avoid failure of the aforementioned validation?

    rkrux commented at 11:30 am on March 13, 2025:
    I am inclining towards not adding a positional argument for this. Mostly because this is not something the user would use frequently and instead would let the defaults take over. I think using an optional options object here (used in many RPCs) would suffice for this use case. @glozow WDYT?

    rkrux commented at 2:57 pm on March 24, 2025:
    Related, see #31936 (review)

    glozow commented at 4:25 pm on March 31, 2025:
    This is needed as the arg is non-string type. My understanding is that @luke-jr is concept nacking the PR. I don’t really understand the comments about the positionalness of this arg?
  12. Bue-von-hon force-pushed on Mar 1, 2025
  13. chungeun-choi commented at 7:23 am on March 1, 2025: none
  14. in test/functional/rpc_rawtransaction.py:266 in db71e16164 outdated
    258@@ -259,7 +259,11 @@ def createrawtransaction_tests(self):
    259         assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [])
    260 
    261         # Test `createrawtransaction` invalid extra parameters
    262-        assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [], {}, 0, False, 'foo')
    263+        assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [], {}, 0, False, 2, 'foo')
    264+
    265+        # Test `createrawtransaction` invalid version parameters
    266+        assert_raises_rpc_error(-8, "Invalid parameter, version out of range(0~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, -1)
    267+        assert_raises_rpc_error(-8, "Invalid parameter, version out of range(0~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, 4)
    


    rkrux commented at 7:32 am on March 1, 2025:
    IMO we should add a test wherein a v3 transaction is successfully created, signed, and broadcast to the network. ATM, only the erroneous cases are covered.

    dongwook-chan commented at 1:31 pm on March 9, 2025:
    @rkrux Utilized existing raw_multisig_transaction_legacy_tests but with version 3 raw transaction. This seems to cover all the domain you specified. Could you pls take a look? https://github.com/bitcoin/bitcoin/blob/1121cb7a91e0dfa4f247273bf89e2e1b47c82628/test/functional/rpc_rawtransaction.py#L96

    rkrux commented at 1:00 pm on March 13, 2025:
  15. rkrux commented at 7:34 am on March 1, 2025: contributor
    Thank you for working on this, please mention in the PR description that it closes the issue #31348.
  16. dongwook-chan commented at 7:37 am on March 1, 2025: none

    @glozow @luke-jr @rkrux

    While updating the code, I noticed a couple of potential improvements.

    1. No range validation tests for locktime argument in createrawtransaction RPC. I added range validation tests for new version argument. Should we extend this to locktime as well?

    2. createpsbt RPC still creates only v2 raw transactions even though it shares same RPCHelpMan with createrawtransaction. Should it support v3 raw transactions too?

  17. DrahtBot removed the label CI failed on Mar 1, 2025
  18. rkrux commented at 8:21 am on March 1, 2025: contributor

    Re: #31936 (comment)

    1. There seems to be couple functional tests related to locktime out of range here: https://github.com/bitcoin/bitcoin/blob/3c1f72a36700271c7c1293383549c3be29f28edb/test/functional/rpc_rawtransaction.py#L312-L313

    2. Good point, I think it should but in a separate PR because along with it, we’d need to ensure that other PSBT related RPCs such as walletcreatefundedpsbt among others work with v3 transactions as well. There’s a PSBT_GLOBAL_TX_VERSION field as well in the PSBT that might need to be set to 3, so I believe best to be in a separate PR.

    This above point makes me realise that related to my previous comment #31936 (review), we can add a more comprehensive functional test for raw transaction related RPCs in this PR that creates a v3 raw transaction createrawtransaction, funds a raw transaction fundrawtransaction, decodes it decoderawtransaction, signs it signrawtransactionwithwallet, and then sends it sendrawtransaction. This would ensure that the integration of v3 with all the related transaction RPCs gel together.

  19. rkrux commented at 9:26 am on March 1, 2025: contributor

    There’s a PSBT_GLOBAL_TX_VERSION field as well in the PSBT that might need to be set to 3

    An update to my previous comment: #31936 (comment)

    This can only done when PSBT V2 is implemented #21283, so no need for this at the moment.

  20. 1440000bytes commented at 4:30 pm on March 3, 2025: none
    Concept ACK
  21. in src/rpc/rawtransaction_util.cpp:159 in db71e16164 outdated
    153@@ -154,6 +154,13 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
    154         rawTx.nLockTime = nLockTime;
    155     }
    156 
    157+    if (!version.isNull()) {
    158+        int64_t nVersion = version.getInt<int64_t>();
    159+        if (nVersion < 0 || nVersion > TX_MAX_STANDARD_VERSION)
    


    instagibbs commented at 7:49 pm on March 6, 2025:
    You should add a test for this, but I’m pretty sure version 0 was never standard for relay.
  22. adamandrews1 commented at 10:41 pm on March 6, 2025: none
    Concept ACK Note: May need to revisit #31298 after this merges
  23. Bue-von-hon force-pushed on Mar 9, 2025
  24. in src/rpc/rawtransaction_util.h:56 in 1121cb7a91 outdated
    53@@ -54,5 +54,6 @@ void AddOutputs(CMutableTransaction& rawTx, const UniValue& outputs_in);
    54 
    55 /** Create a transaction from univalue parameters */
    56 CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, std::optional<bool> rbf);
    


    maflcko commented at 11:06 am on March 9, 2025:
    not sure about default args. It would be better to explicitly enumerate the call sites and just put the default there, if needed.

    rkrux commented at 12:38 pm on March 13, 2025:
    Is this comment outdated? I don’t understand where are the default args here that are referred to in the above comment.

    maflcko commented at 11:29 am on March 14, 2025:

    Is this comment outdated? I don’t understand where are the default args here that are referred to in the above comment.

    In C++, function overloading can be used as a tool to set default values for function parameters. This is done here, by setting the default value in the function implementation, as opposed to in the function signature.


    rkrux commented at 12:01 pm on March 14, 2025:
    Oh I see this comment referred to the new implementation of this older function caused by the introduction of function overloading here. Yeah, I am not in favour of this as well, seem unnecessary for just a version param, the function signature defaults can suffice.

    maflcko commented at 12:20 pm on March 14, 2025:

    I was trying to say in #31936 (review) that a default doesn’t make sense at all here (in any way), because:

    • Every call site that omits the default has the same fallback, so changing the default in the future will make review harder.
    • Every call site needs to document the default in the RPC help anyway, so simply using the default from the docs is self-consistent and self-documenting and avoids the need for a default here

    Finally, default values in C++ function signatures or function overloads are problematic, when more defaults are added in the future, because there is no way to skip over one default value and only specify the other one. Modern languages, such as Rust don’t support this at all, or Python allows for named args.


    rkrux commented at 2:19 pm on March 24, 2025:

    Interesting, I like these points quite a lot. Summarising below, please correct me if I misunderstand:

    We can avoid using defaults here completely by letting the corresponding version default trickle down from the RPC help into these functions here.

    As I checked in the call sites of CreateTransaction, this^ requires other transaction creation RPCs to accept a version param as well such as createpsbt, walletcreatefundedpsbt, send, sendall, which seems consistent with an earlier suggestion here #31936 (review).

    This increases the scope of the PR a bit but I feel it’s worth doing these changes as these RPCs are quite related to each other in terms of functionality. I can correspondingly update the issue description as well.

    This also helps to make a case for using the options argument in a previously open ended question here #31936 (review) because most of the above mentioned RPCs accept an option param wherein the version option can be added.


    Bue-von-hon commented at 8:33 am on April 5, 2025:
    This is truly amazing content. I wrote a blog post on this topic.
  25. in test/functional/rpc_rawtransaction.py:499 in 1121cb7a91 outdated
    495@@ -491,7 +496,7 @@ def transaction_version_number_tests(self):
    496         decrawtx = self.nodes[0].decoderawtransaction(rawtx)
    497         assert_equal(decrawtx['version'], 0xffffffff)
    498 
    499-    def raw_multisig_transaction_legacy_tests(self):
    500+    def raw_multisig_transaction_legacy_tests(self, version=2):
    


    rkrux commented at 12:28 pm on March 13, 2025:

    Although this works but there seems to be a note here regarding legacy wallets, which are being deprecated. I think a more suitable place could be in the getrawtransaction_verbosity_tests that use create_self_transfer(), which already accepts a version argument in the tests - just need to pass down 3.

    Also, testing the value of the transaction version after being mined would complete the all-round testing of this functionality for which only adding a new field for version in the getrawtransaction_verbosity_tests subtest should suffice here: https://github.com/bitcoin/bitcoin/blob/199d47d9629b603d5a23b96c5260c68add00ccb3/test/functional/rpc_rawtransaction.py#L187

  26. in src/rpc/client.cpp:184 in 1121cb7a91 outdated
    180@@ -180,6 +181,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
    181     { "createpsbt", 1, "outputs" },
    182     { "createpsbt", 2, "locktime" },
    183     { "createpsbt", 3, "replaceable" },
    184+    { "createpsbt", 4, "version" },
    


    rkrux commented at 12:36 pm on March 13, 2025:

    If accepting a new argument can’t be avoided for this PSBT RPC because of the same CreateTxDoc() being reused underneath, then I think we can explore updating functional tests for PSBT in this PR itself because createpsbt RPC uses the same ConstructTransaction() as well.

    rpc_psbt.py is the file that would need to be updated. Using a similar approach like done in the rpc_rawtransaction would be preferred that is reusing bunch of the existing testing methods.

  27. rkrux commented at 2:20 pm on March 24, 2025: contributor
    Answering open comments.
  28. maflcko commented at 6:27 am on March 31, 2025: member
    Are you still working on this?
  29. Bue-von-hon commented at 7:05 am on March 31, 2025: none

    Are you still working on this?

    sure!

  30. in src/policy/policy.h:147 in 1121cb7a91 outdated
    143@@ -144,6 +144,7 @@ std::vector<uint32_t> GetDust(const CTransaction& tx, CFeeRate dust_relay_rate);
    144 // Changing the default transaction version requires a two step process: first
    145 // adapting relay policy by bumping TX_MAX_STANDARD_VERSION, and then later
    146 // allowing the new transaction version in the wallet/RPC.
    147+static constexpr decltype(CTransaction::version) TX_MIN_STANDARD_VERSION{1};
    


    glozow commented at 2:57 pm on March 31, 2025:
    concept ack introducing this constant. I think it would be best to have it in a separate commit and replace the magic number in policy.cpp with this

    Bue-von-hon commented at 7:40 am on April 5, 2025:
    I’m not sure about the meaning of ‘separate commit,’ but I replaced the magic number in the policy.cpp file with a constant value.

    glozow commented at 7:25 pm on April 7, 2025:
    I mean putting the changes assigning 1 to this constant in one commit, and then the version parameter addition in a second commit.
  31. in src/rpc/rawtransaction.cpp:161 in 1121cb7a91 outdated
    157@@ -158,6 +158,7 @@ static std::vector<RPCArg> CreateTxDoc()
    158         {"locktime", RPCArg::Type::NUM, RPCArg::Default{0}, "Raw locktime. Non-0 value also locktime-activates inputs"},
    159         {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{true}, "Marks this transaction as BIP125-replaceable.\n"
    160                 "Allows this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible."},
    161+        {"version", RPCArg::Type::NUM, RPCArg::Default{2}, "Transaction version"},
    


    glozow commented at 2:58 pm on March 31, 2025:
    Should use CURRENT_VERSION instead of magic number

    Bue-von-hon commented at 8:42 am on April 5, 2025:
    Fix it.
  32. fanquake marked this as a draft on Apr 1, 2025
  33. Bue-von-hon force-pushed on Apr 5, 2025
  34. Bue-von-hon commented at 11:36 am on April 5, 2025: none

    Resolves code review. The following tests are failing, but I will fix them soon.

     0rpc_psbt.py --descriptors                                            |  Failed  | 12 s
     1tool_wallet.py --descriptors                                         |  Failed  | 6 s
     2wallet_bumpfee.py --descriptors                                      |  Failed  | 11 s
     3wallet_create_tx.py --descriptors                                    |  Failed  | 2 s
     4wallet_fundrawtransaction.py --descriptors                           |  Failed  | 5 s
     5wallet_miniscript.py --descriptors                                   |  Failed  | 1 s
     6wallet_resendwallettransactions.py --descriptors                     |  Failed  | 13 s
     7wallet_send.py --descriptors                                         |  Failed  | 14 s
     8wallet_spend_unconfirmed.py                                          |  Failed  | 1 s
     9wallet_taproot.py --descriptors                                      |  Failed  | 8 s
    10wallet_txn_clone.py                                                  |  Failed  | 1 s
    11wallet_txn_clone.py --mineblock                                      |  Failed  | 1 s
    12wallet_txn_clone.py --segwit                                         |  Failed  | 1 s
    13wallet_txn_doublespend.py --descriptors                              |  Failed  | 1 s
    14wallet_txn_doublespend.py --mineblock                                |  Failed  | 1 s
    
  35. Bue-von-hon marked this as ready for review on Apr 5, 2025
  36. DrahtBot commented at 12:33 pm on April 5, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/40031572119

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  37. DrahtBot added the label CI failed on Apr 5, 2025
  38. in src/wallet/rpc/spend.cpp:1298 in 3722e108ae outdated
    1294@@ -1295,7 +1295,7 @@ RPCHelpMan send()
    1295                     ParseOutputs(outputs),
    1296                     InterpretSubtractFeeFromOutputInstructions(options["subtract_fee_from_outputs"], outputs.getKeys())
    1297             );
    1298-            CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf);
    1299+            CMutableTransaction rawTx = ConstructTransaction(options["inputs"], request.params[0], options["locktime"], rbf, 3);
    


    glozow commented at 7:33 pm on April 7, 2025:
    You are changing the default version without motivation here. Also, as mentioned in previous review comments, please try to avoid magic numbers - here and in 3 other places.
  39. glozow commented at 7:35 pm on April 7, 2025: member
    CI is still failing
  40. Bue-von-hon force-pushed on Apr 11, 2025
  41. Bue-von-hon commented at 0:14 am on April 11, 2025: none

    CI is still failing @glozow Resolves Code Review! PTAL!! I have created a new commit to add the TX_MIN_VERSION parameter. I am concerned that this change violates the rule of creating only one commit per PR. The tests were failing because spend.cpp was using version 3, while rawtransaction.cpp was using version 2. I fixed the test failures by modifying both to use version 2. If you want to use version 3 of rawtransaction, it should be possible since the method accepts the version as a parameter. There is no constant value to replace version 2, so the use of magic numbers has been retained.

  42. Bue-von-hon marked this as a draft on Apr 11, 2025
  43. rkrux commented at 7:49 am on April 11, 2025: contributor

    Re: #31936 (comment)

    I am concerned that this change violates the rule of creating only one commit per PR.

    I don’t think this is true because this rule seems quite limiting, and there are many PRs with more than one commit. Is this mentioned somewhere in the repo? We can get it updated separately.

  44. Bue-von-hon commented at 10:52 am on April 11, 2025: none

    Re: #31936 (comment)

    I am concerned that this change violates the rule of creating only one commit per PR.

    I don’t think this is true because this rule seems quite limiting, and there are many PRs with more than one commit. Is this mentioned somewhere in the repo? We can get it updated separately.

    I misunderstood. I thought the concept of “atomic commits” meant having one commit per PR. I didn’t realize it meant “you shouldn’t modify the same line across multiple commits. link

  45. Bue-von-hon force-pushed on Apr 11, 2025
  46. Bue-von-hon force-pushed on Apr 11, 2025
  47. Bue-von-hon force-pushed on Apr 11, 2025
  48. Bue-von-hon force-pushed on Apr 11, 2025
  49. Bue-von-hon force-pushed on Apr 11, 2025
  50. Bue-von-hon marked this as ready for review on Apr 11, 2025
  51. Bue-von-hon marked this as a draft on Apr 11, 2025
  52. Bue-von-hon marked this as ready for review on Apr 11, 2025
  53. Bue-von-hon commented at 2:14 pm on April 11, 2025: none

    @glozow To avoid using magic numbers, I used the CTransaction::CURRENT_VERSION constant, but this change caused the CI to fail. I think we might need to add a commit to introduce a new constant. What do you think?

    [20:44:30.954] /ci_container_base/ci/scratch/build-i686-pc-linux-gnu/src/./rpc/rawtransaction.cpp:161:(.text+0x29162): undefined reference to CTransaction::CURRENT_VERSION'`

  54. DrahtBot removed the label CI failed on Apr 11, 2025
  55. Bue-von-hon requested review from luke-jr on Apr 11, 2025
  56. Bue-von-hon requested review from instagibbs on Apr 11, 2025
  57. Bue-von-hon requested review from maflcko on Apr 11, 2025
  58. Bue-von-hon requested review from rkrux on Apr 11, 2025
  59. Bue-von-hon requested review from glozow on Apr 11, 2025
  60. Bue-von-hon requested review from adamandrews1 on Apr 11, 2025
  61. Bue-von-hon commented at 10:59 am on April 15, 2025: none
  62. glozow commented at 6:14 pm on April 15, 2025: member

    @glozow To avoid using magic numbers, I used the CTransaction::CURRENT_VERSION constant, but this change caused the CI to fail.

    You can declare a DEFAULT_RAWTX_VERSION{CTransaction::CURRENT_VERSION}

    Also, I think you should swap the commits: first assign the minimum standard version to a named constant, and then use it. Currently, you are introducing a magic number and then renaming it to MIN_VERSION - that seems backwards. This is a nit, but this order defeats the purpose of having a separate commit imo.

  63. w0xlt commented at 8:31 pm on April 17, 2025: contributor

    Changes made to rpc_rawtransaction.py in https://github.com/bitcoin/bitcoin/pull/31936/commits/d0453049f5d6678096f3575f6d595467506a05f1 in the functions raw_multisig_transaction_legacy_tests and raw_multisig_transaction_legacy_tests does not have any effect.

    A better approach might be to validate a v3 transaction in createrawtransaction_tests, for example, as shown below.

     0        # Multiple mixed outputs
     1        tx = tx_from_hex(self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=multidict([(address, 99), (address2, 99), ('data', '99')])))
     2+       assert_equal(tx.version, 2)
     3        assert_equal(len(tx.vout), 3)
     4        assert_equal(
     5            tx.serialize().hex(),
     6            self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=[{address: 99}, {address2: 99}, {'data': '99'}]),
     7        )
     8
     9+       rawtx_v3 = self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=OrderedDict([(address, 99), (address2, 99)]), version=3)
    10+       tx = tx_from_hex(rawtx_v3)
    11+       assert_equal(tx.version, 3)
    
  64. DrahtBot added the label Needs rebase on Apr 25, 2025
  65. Bue-von-hon force-pushed on Apr 26, 2025
  66. rpc: Add version parameter for transaction 51c7b4c9e0
  67. Bue-von-hon marked this as a draft on Apr 26, 2025
  68. rpc: Add current version parameter for transaction cb15291ea2
  69. rpc: Support v3 raw transactions creation
    Added support for creating v3 raw transaction:
    - Overloaded  to include additional parameter
    
    Co-authored-by: chungeun-choi <cucuridas@gmail.com>
    Co-authored-by: dongwook-chan <dongwook.chan@gmail.com>
    Co-authored-by: sean-k1 <uhs2000@naver.com>
    a43237d11e
  70. Bue-von-hon force-pushed on Apr 26, 2025
  71. Fix test 63d9b9e3b8
  72. Bue-von-hon marked this as ready for review on Apr 26, 2025
  73. Bue-von-hon commented at 12:30 pm on April 26, 2025: none
  74. DrahtBot removed the label Needs rebase on Apr 26, 2025
  75. Bue-von-hon commented at 5:13 am on April 30, 2025: none
  76. in src/rpc/rawtransaction_util.cpp:160 in 63d9b9e3b8
    153@@ -154,6 +154,13 @@ CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniVal
    154         rawTx.nLockTime = nLockTime;
    155     }
    156 
    157+    if (!version.isNull()) {
    158+        uint32_t nVersion = version.getInt<uint32_t>();
    159+        if (nVersion < TX_MIN_STANDARD_VERSION || nVersion > TX_MAX_STANDARD_VERSION)
    160+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, version out of range(1~3)");
    


    adamandrews1 commented at 4:51 pm on May 1, 2025:
    This RPC error should be a formatted string so that it remains accurate as TX_MIN_STANDARD_VERSION and TX_MAX_STANDARD_VERSION get updated.
  77. in test/functional/rpc_rawtransaction.py:262 in 63d9b9e3b8
    254@@ -255,7 +255,11 @@ def createrawtransaction_tests(self):
    255         assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [])
    256 
    257         # Test `createrawtransaction` invalid extra parameters
    258-        assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [], {}, 0, False, 'foo')
    259+        assert_raises_rpc_error(-1, "createrawtransaction", self.nodes[0].createrawtransaction, [], {}, 0, False, 2, 3, 'foo')
    260+
    261+        # Test `createrawtransaction` invalid version parameters
    262+        assert_raises_rpc_error(-8, "Invalid parameter, version out of range(1~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, 0)
    263+        assert_raises_rpc_error(-8, "Invalid parameter, version out of range(1~3)", self.nodes[0].createrawtransaction, [], {}, 0, False, 4)
    


    adamandrews1 commented at 4:53 pm on May 1, 2025:
    The final parameter (tx version) for this test should be set to TX_MAX_STANDARD_VERSION + 1 or at least to a number with headroom like 999.
  78. in test/functional/rpc_rawtransaction.py:344 in 63d9b9e3b8
    338@@ -335,6 +339,10 @@ def createrawtransaction_tests(self):
    339             self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=[{address: 99}, {address2: 99}, {'data': '99'}]),
    340         )
    341 
    342+        rawtx_v3 = self.nodes[2].createrawtransaction(inputs=[{'txid': TXID, 'vout': 9}], outputs=OrderedDict([(address, 99), (address2, 99)]), version=3)
    343+        tx = tx_from_hex(rawtx_v3)
    344+        assert_equal(tx.version, 3)
    


    adamandrews1 commented at 4:56 pm on May 1, 2025:
    Shouldn’t this test be repeated for each valid tx.version in the range [TX_MIN_STANDARD_VERSION, TX_MAX_STANDARD_VERSION]?
  79. adamandrews1 commented at 4:57 pm on May 1, 2025: none

    utACK 63d9b9e3b8e225eccd55209e7a4213604ec88425

    Suggested some improvements, but not blocking ones IMO

  80. DrahtBot requested review from glozow on May 1, 2025
  81. achow101 commented at 0:42 am on May 3, 2025: member

    Approach NACK

    While I agree that we should support v3 transactions, I think this implementation is incomplete without any v3 support in the wallet’s coin selection. As it is now, this appears to enable a massive footgun - users can use createrawtransaction to create a v3 transaction, fund it with fundrawtransaction, and then wind up with a v3 transaction that they cannot broadcast. This is because the topological restrictions of v3 transactions are not being taken into account during coin selection, it is possible that the wallet will select inputs which are incompatible with a v3 transaction, and therefore produce a transaction that won’t be broadcast.

    I expect that supporting sending v3 transactions from Bitcoin Core will actual be a significantly more involved project. While I appreciate your enthusiasm, I think it would be better to leave the implementation of this feature to experienced contributors.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-05-08 21:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me