Opt-in RBF must be strictly enabled or disabled before GBT can be called #7464

pull wangchun wants to merge 1 commits into bitcoin:master from wangchun:master changing 4 files +13 −0
  1. wangchun commented at 10:06 pm on February 3, 2016: none
    Pool operators must strictly specify their preference to opt-in RBF.
  2. luke-jr commented at 10:07 pm on February 3, 2016: member
    Concept ACK, but see #3229 for past discussion.
  3. dcousens commented at 10:34 pm on February 3, 2016: contributor
    Concept ACK, light utACK 07bd7c5
  4. jtoomim commented at 10:56 pm on February 3, 2016: none
    This probably breaks unit tests. Perhaps you should add a check for regtest or testnet mode first? #3229 had the same issue.
  5. Require configuration of opt-in RBF before creating blocks 02bc16ab0e
  6. wangchun force-pushed on Feb 4, 2016
  7. wangchun commented at 1:03 am on February 4, 2016: none
    Updated with check for testnet and regtest
  8. laanwj added the label Mining on Feb 4, 2016
  9. sandakersmann commented at 2:36 pm on February 4, 2016: contributor
    ACK
  10. laanwj commented at 2:41 pm on February 4, 2016: member
    It would be better to update the RPC tests to pass the option explicitly instead of adding a flag to CChainParams. This makes sure that the switching code is (or at least, can be) actually tested.
  11. sandakersmann commented at 2:41 pm on February 4, 2016: contributor
    This should be in 0.12
  12. luke-jr commented at 6:05 pm on February 4, 2016: member

    Agree with @laanwj on passing the flag in tests.

    I also think it would be better to just reopen #3229 if the NACKs there are determined to be outdated/overruled… @sandakersmann Way too late for any changes to 0.12 now.

  13. sandakersmann commented at 6:10 pm on February 4, 2016: contributor
    @luke-jr Then we should set default for -mempoolreplacement to false in 0.12.
  14. petertodd commented at 7:50 pm on February 4, 2016: contributor

    Weak NACK

    I’m dubious about making it more difficult for users to mine out-of-the-box. p2pool is a thing that we want to encourage, and every additional setting that you must set to use it is a major barrier to adoption; there are literally dozens of settings that you might want to set.

    A better way to deal with this kind of choice is choice between different Bitcoin protocol implementations with different philosophies behind them, e.g. Bitcoin Core and Bitcoin Classic.

    Also, for this setting in particular, there was very little controversy associated with it pre-merging, including ACK’s by Jeff Garzik and others with strong pro-zeroconf feelings. This is notably unlike things like OP_RETURN, where even among the technical community there was controversy pre-merging. The controversy post-merge seems to be based mainly on misunderstandings of how well zeroconf works; note my blog post on the subject: https://petertodd.org/2016/are-wallets-ready-for-rbf

  15. sandakersmann commented at 10:24 pm on February 4, 2016: contributor
    @petertodd 0-conf works well enough today. Too bad you have to destroy this use case and make bitcoin less useful as a currency.
  16. gmaxwell commented at 10:30 pm on February 4, 2016: contributor
    @sandakersmann Please treat contributors here with respect. None of the contributors here want to make Bitcoin less useful. It sounds like you may be confused about the functionality in Bitcoin Core, please check out the FAQ: https://bitcoincore.org/en/faq/optin_rbf/
  17. sandakersmann commented at 10:42 pm on February 4, 2016: contributor
    @gmaxwell I have already checked out the FAQ and I’m not confused. When it comes to showing respect I’m sure I’m not the worst around here.
  18. luke-jr commented at 11:53 pm on February 4, 2016: member
    @sandakersmann If you are not confused, then you should be aware that RBF does not break unconfirmed/off-chain transactions at all. They continue to work fine with the same lack of security they already have. Anyhow, this is not the place to discuss the merits of specific policies.
  19. Aquentus commented at 8:16 pm on February 7, 2016: none
    utACK. Developers, with no modern mining or economic experience, should not be making policy choices in regards to defaults anyway, but only offer options so that the miners can choose policy aspects in a decentralized manner.
  20. in src/chainparams.cpp: in 02bc16ab0e
    243@@ -242,6 +244,7 @@ class CRegTestParams : public CChainParams {
    244         vSeeds.clear();  //! Regtest mode doesn't have any DNS seeds.
    245 
    246         fMiningRequiresPeers = false;
    247+        fMiningRequiresConfiguration = false;
    


    MarcoFalke commented at 10:34 am on February 9, 2016:

    Please remove those. Each difference between regnet and mainnet makes our tests a bit less useful.

    You can adjust this in qa/rpc-tests/test_framework/util.py or even better in the respective qa/rpc-tests that cover this.

  21. laanwj commented at 4:00 pm on April 1, 2016: member
    Closing this, there is no agreement to do this and furthermore, comments on the code are not being addressed.
  22. laanwj closed this on Apr 1, 2016

  23. DrahtBot locked this on Sep 8, 2021

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: 2024-10-30 03:12 UTC

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