wallet, RPC: Default BIP125 signal to -mempoolfullrbf #25602

1. 
   MarcoFalke commented at 9:04 am on July 13, 2022: member

   -mempoolfullrbf allows valid replacement transactions into the mempool, even if they do not signal for BIP125. If it is set, it would be confusing if the wallet and RPC didn’t signal for BIP125 replacement:

   • The wallet will refuse to replace non-signalling transactions.
   • However, such transactions may be replaced over RPC or P2P.
   • Such replacements may not propagate well over the P2P network.

   So to prevent those issues in the normal case, adjust the default for -walletrbf as well as the RPC raw transactions API.

   It is still possible for users to set -mempoolfullrbf=1 -walletrbf=0, as well as passing in the replaceable=False option to each RPC, if they wish. Or to leave the -mempoolfullrbf default value: off.

2. 
   darosior commented at 9:09 am on July 13, 2022: member

   Concept ACK
3. fanquake requested review from instagibbs on Jul 13, 2022
4. fanquake requested review from ariard on Jul 13, 2022
5. fanquake requested review from glozow on Jul 13, 2022
6. wallet: Default -walletrbf to -mempoolfullrbf fa0ed59467
7. rpc: Default raw txs replaceable option to -mempoolfullrbf fac6ce45bc
8. MarcoFalke force-pushed on Jul 13, 2022
9. glozow requested review from achow101 on Jul 13, 2022
10. 
    glozow commented at 12:26 pm on July 13, 2022: member

    Concept ACK based on the description, will review more closely soon
11. MarcoFalke added the label Wallet on Jul 13, 2022
12. MarcoFalke added the label RPC/REST/ZMQ on Jul 13, 2022
13. 
    achow101 commented at 7:49 pm on July 13, 2022: member

    Tend to NACK

    I would expect that -walletrbf would default to 1 sooner than -mempoolfullrbf will default to 1, so having these tied just makes it more annoying to make that change. Otherwise, I agree that it would make sense to default -walletrbf=1 when -mempoolfullrbf=1.

14. in src/rpc/rawtransaction.cpp:309 in fac6ce45bc

    308 
    309-    bool rbf = false;
    310-    if (!request.params[3].isNull()) {
    311-        rbf = request.params[3].isTrue();
    312-    }
    313+    const bool rbf{request.params[3].isNull() ? args.GetBoolArg("-mempoolfullrbf", DEFAULT_MEMPOOL_FULL_RBF) : request.params[3].getBool()};
    

    ---

    ---

    

    ariard commented at 0:06 am on July 14, 2022:

    I think the following works, though I would rather do some wallet options encapsulations like done with MemPoolOptions. I believe the wallet could have its own default transaction-relay policy settings in case the mempool ones are bigger (e.g if local limitdescendantsize=100, still makes sense to propagate chain of transactions of max size=25)

     0diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
     1index 263baa3c9..dc9f02160 100644
     2--- a/src/rpc/rawtransaction.cpp
     3+++ b/src/rpc/rawtransaction.cpp
     4@@ -305,8 +305,9 @@ static RPCHelpMan createrawtransaction()
     5         }, true
     6     );
     7     const ArgsManager& args{EnsureAnyArgsman(request.context)};
     8+    const NodeContext& node = EnsureAnyNodeContext(request.context);
     9 
    10-    const bool rbf{request.params[3].isNull() ? args.GetBoolArg("-mempoolfullrbf", DEFAULT_MEMPOOL_FULL_RBF) : request.params[3].getBool()};
    11+    const bool rbf{request.params[3].isNull() ? node.mempool->m_full_rbf : request.params[3].getBool()};
    12     CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], rbf);
    13 
    14     return EncodeHexTx(CTransaction(rawTx));
    

15. 
    ariard commented at 0:07 am on July 14, 2022: member

    Well I think #25610 is a better approach rather than coupling wallet and mempool settings.
16. 
    DrahtBot commented at 3:55 am on July 14, 2022: contributor

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25610 (wallet, rpc: Opt in to RBF by default 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.

17. MarcoFalke closed this on Jul 14, 2022

    ---

18. MarcoFalke deleted the branch on Jul 14, 2022
19. bitcoin locked this on Jul 14, 2023

Contributors
MarcoFalke darosior glozow achow101 ariard DrahtBot

Review Requested
achow101 instagibbs

Labels
Wallet RPC/REST/ZMQ