doc: Bip70 removal follow-up #17285

fjahr commented at 6:47 PM on October 28, 2019: member

A small follow-up to #17165 which removed BIP70 support.

Removes one leftover mention of BIP70 in a comment.
Removes BIP70 reference in comments on network/chain name strings. These can be removed as they are not really helpful and also incorrect: BIP70 only defines "main" and "test" but not "regtest". If/When signet gets merged we will add another name to the list that is not defined in BIP70. Mostly there is also an exhaustive list of the options included in the comment anyway.

If we would like to keep an identifier for this naming scheme, I would suggest switching to something more generic, like 'short chain name'. Happy to implement that if that is preferred. Alternatively, we could add a reference to CBaseChainParams. That would also mean we don't have to change these lines again for signet.

fanquake added the label Docs on Oct 28, 2019

laanwj commented at 7:46 PM on October 28, 2019: member

Removes BIP70 reference in comments on network/chain name strings

I was hesitant about this at first, but tend to agree nothing is lost by simply calling it "chain name". They are part of the RPC API (not only internal identifier) so this doesn't mean it can be changed willy-nilly from now on, though.

DrahtBot commented at 8:01 PM on October 28, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16411 (Signet support by kallewoof)

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.

MarcoFalke commented at 8:05 PM on October 28, 2019: member

Concept ACK

in src/qt/walletmodel.h:68 in 3f5be66b6c outdated

  63 | @@ -64,7 +64,7 @@ class SendCoinsRecipient
  64 |      CAmount amount;
  65 |      // If from a payment request, this is used for storing the memo
  66 |      QString message;
  67 | -    // If building with BIP70 is disabled, keep the payment request around as
  68 | +    // The payment request needs to be kept around as
  69 |      // serialized string to ensure load/store is lossless

jonatack commented at 10:34 AM on October 29, 2019:

Suggestion:

    // Keep the payment request around as a serialized string to ensure
    // load/store is lossless.

e.g. notably s/as/as a/

fjahr commented at 4:54 PM on October 29, 2019:

Took your suggestion. Thanks!

jonatack commented at 10:41 AM on October 29, 2019: member

ACK 3f5be66b6c4466a72c2409fe53d2a2da01c87702

Feel free to ignore the suggestion below. Note that since all changes are documentation ones, the commit prefixed with "wallet" could be amended to "doc".

wallet: Remove left-over BIP70 comment d6e493f0c2

fjahr force-pushed on Oct 29, 2019

jachiang commented at 2:28 PM on October 30, 2019: contributor

ACK b413bdf6b

in src/chainparamsbase.h:20 in b413bdf6b0 outdated

  14 | @@ -15,7 +15,7 @@
  15 |  class CBaseChainParams
  16 |  {
  17 |  public:
  18 | -    /** BIP70 chain name strings (main, test or regtest) */
  19 | +    /** Chain name strings (main, test or regtest) */
  20 |      static const std::string MAIN;

MarcoFalke commented at 3:00 PM on October 30, 2019:

Wrong doxygen comment: https://dev.visucore.com/bitcoin/doxygen/class_c_base_chain_params.html#ae2c5dfdbbff4d5f92948258a7b4db47b

This is tied to MAIN

laanwj commented at 3:26 PM on October 30, 2019:

Maybe a doxygen grouping would be useful here: http://www.doxygen.nl/manual/grouping.html

fjahr commented at 11:10 AM on November 1, 2019:

done, with grouping

in src/chainparams.h:78 in b413bdf6b0 outdated

  74 | @@ -75,7 +75,7 @@ class CChainParams
  75 |      uint64_t AssumedChainStateSize() const { return m_assumed_chain_state_size; }
  76 |      /** Whether it is possible to mine blocks on demand (no retargeting) */
  77 |      bool MineBlocksOnDemand() const { return consensus.fPowNoRetargeting; }
  78 | -    /** Return the BIP70 network string (main, test or regtest) */
  79 | +    /** Return the network string (main, test or regtest) */

MarcoFalke commented at 3:01 PM on October 30, 2019:

    /** Return the network string */

Could be removed? This way it doesn't have to be updated for signet again

fjahr commented at 11:10 AM on November 1, 2019:

done

in src/util/system.h:268 in b413bdf6b0 outdated

 264 | @@ -265,7 +265,7 @@ class ArgsManager
 265 |      void ForceSetArg(const std::string& strArg, const std::string& strValue);
 266 |  
 267 |      /**
 268 | -     * Looks for -regtest, -testnet and returns the appropriate BIP70 chain name.
 269 | +     * Looks for -regtest, -testnet and returns the appropriate chain name.

MarcoFalke commented at 3:03 PM on October 30, 2019:

     * Returns the appropriate chain name from the program arguments.

or similar

fjahr commented at 11:10 AM on November 1, 2019:

done

fanquake added the label Waiting for author on Oct 30, 2019

doc: Remove explicit network name references 3ed8e3d079

fjahr force-pushed on Nov 1, 2019

fanquake removed the label Waiting for author on Nov 1, 2019

MarcoFalke commented at 12:51 PM on November 1, 2019: member

ACK 3ed8e3d079a3860dcdf944f7c1aa37765a53da32

laanwj referenced this in commit 463eab5e14 on Nov 2, 2019

laanwj merged this on Nov 2, 2019

laanwj closed this on Nov 2, 2019

sidhujag referenced this in commit 902791dd2a on Nov 3, 2019

sidhujag referenced this in commit 50ec6bafea on Nov 10, 2020

MarcoFalke locked this on Dec 16, 2021