BIP-325: Signet [consensus] #18267

pull kallewoof wants to merge 10 commits into bitcoin:master from kallewoof:2003-signet-consensus changing 21 files +532 −44
  1. kallewoof commented at 7:16 am on March 5, 2020: member

    This PR is a part of BIP-325 (https://github.com/bitcoin/bips/blob/master/bip-0325.mediawiki), and is a sub-PR of #16411.

    • Signet consensus (this)
    • Signet RPC tools (pending)
    • Signet utility scripts (contrib/signet) (pending)
  2. kallewoof force-pushed on Mar 5, 2020
  3. kallewoof force-pushed on Mar 5, 2020
  4. kallewoof force-pushed on Mar 5, 2020
  5. kallewoof force-pushed on Mar 5, 2020
  6. kallewoof force-pushed on Mar 5, 2020
  7. DrahtBot added the label Build system on Mar 5, 2020
  8. DrahtBot added the label Consensus on Mar 5, 2020
  9. DrahtBot added the label GUI on Mar 5, 2020
  10. DrahtBot added the label Tests on Mar 5, 2020
  11. DrahtBot added the label Utils/log/libs on Mar 5, 2020
  12. DrahtBot added the label Validation on Mar 5, 2020
  13. kallewoof force-pushed on Mar 5, 2020
  14. DrahtBot commented at 3:30 pm on March 5, 2020: 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:

    • #19937 (signet mining utility by ajtowns)
    • #19438 (Introduce deploymentstatus by ajtowns)

    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.

  15. laanwj added this to the "Blockers" column in a project

  16. practicalswift commented at 9:43 pm on March 5, 2020: contributor
    Concept ACK
  17. kallewoof force-pushed on Mar 6, 2020
  18. DrahtBot added the label Needs rebase on Mar 11, 2020
  19. kallewoof force-pushed on Mar 12, 2020
  20. DrahtBot removed the label Needs rebase on Mar 12, 2020
  21. DrahtBot added the label Needs rebase on Mar 13, 2020
  22. jonatack commented at 8:34 pm on March 22, 2020: member
    Concept ACK. Local build and tests green, will review shortly.
  23. kallewoof force-pushed on Mar 24, 2020
  24. kallewoof force-pushed on Mar 24, 2020
  25. DrahtBot removed the label Needs rebase on Mar 24, 2020
  26. DrahtBot added the label Needs rebase on Mar 27, 2020
  27. laanwj removed the label Build system on Mar 27, 2020
  28. laanwj removed the label GUI on Mar 27, 2020
  29. laanwj removed the label Tests on Mar 27, 2020
  30. laanwj removed the label Utils/log/libs on Mar 27, 2020
  31. kallewoof force-pushed on Mar 30, 2020
  32. DrahtBot removed the label Needs rebase on Mar 30, 2020
  33. in src/primitives/block.cpp:39 in 79865a6d28 outdated
    34+    int cidx = GetWitnessCommitmentIndex();
    35+    if (cidx == -1) return false;
    36+    auto script = vtx.at(0)->vout.at(cidx).scriptPubKey;
    37+    opcodetype opcode;
    38+    CScript::const_iterator pc = script.begin();
    39+    ++pc; // move beyond initial OP_RETURN
    


    instagibbs commented at 1:58 pm on April 8, 2020:

    shouldn’t we at least assert that the length of the script is > 0?

    a normal script.GetOp and checking the result seems more self-explanatory and safe to me


    kallewoof commented at 3:46 am on April 9, 2020:
    I don’t think it’s strictly necessary as the witness commitment index search requires an OP_RETURN, but I don’t think it hurts either so will switch.
  34. instagibbs commented at 2:17 pm on April 8, 2020: member
    concept ACK, scope ACK, if I can actually connect to signet(still waiting for connections from DNS response)
  35. instagibbs commented at 2:18 pm on April 8, 2020: member

    bitcoin-qt: qt/bitcoin.cpp:528: int GuiMain(int, char**): Assertion `!networkStyle.isNull()’ failed.

    oops

  36. kallewoof commented at 3:42 am on April 9, 2020: member

    bitcoin-qt: qt/bitcoin.cpp:528: int GuiMain(int, char**): Assertion `!networkStyle.isNull()’ failed.

    oops

    Oh.. Yeah I wanted to keep this ultra minimal, but I should probably not crash QT, yeah… :)

  37. kallewoof force-pushed on Apr 9, 2020
  38. kallewoof commented at 4:19 am on April 9, 2020: member

    concept ACK, scope ACK, if I can actually connect to signet(still waiting for connections from DNS response)

    Weird. I just now started a fresh instance with a new datadir and it instantly connected and started syncing blocks..

  39. DrahtBot added the label Needs rebase on Apr 10, 2020
  40. kallewoof force-pushed on Apr 11, 2020
  41. DrahtBot removed the label Needs rebase on Apr 11, 2020
  42. kallewoof force-pushed on Apr 11, 2020
  43. jonatack commented at 6:26 pm on April 27, 2020: member
    Signet worked perfectly the first try :100: …following the instructions at https://gist.github.com/kallewoof/98b6d8dbe126d2b6f47da0ddccd2aa5a… now reviewing
  44. jonatack commented at 12:18 pm on April 28, 2020: member

    Er, I was excited (e.g. “this is so cool”) and should have been more specific.

     0cd src
     1mkdir signet
     2echo "signet=1" > signet/bitcoin.conf
     3
     4./bitcoind -datadir=signet  # in a separate terminal buffer to watch the debug log
     5
     6./bitcoin-cli -datadir=signet -getinfo
     7
     8./bitcoin-cli -datadir=signet getnewaddress
     9sb1q77gyzqu9fawygs9suvyfaga2f4mn9kmz2yudk0
    10
    11./bitcoin-cli -datadir=signet setlabel sb1q77gyzqu9fawygs9suvyfaga2f4mn9kmz2yudk0 testing-signet
    12
    13./bitcoin-cli -datadir=signet getaddressinfo sb1q77gyzqu9fawygs9suvyfaga2f4mn9kmz2yudk0
    14{
    15  "address": "sb1q77gyzqu9fawygs9suvyfaga2f4mn9kmz2yudk0",
    16  "scriptPubKey": "0014f7904103854f5c4440b0e3089ea3aa4d7732db62",
    17  "ismine": true,
    18  "solvable": true,
    19  "desc": "wpkh([6352002d/0'/0'/0']028f681e25caadc71f45e7811775f4e73b5b27ce7bf335888d514a9fc3999c55c1)#de6xjlkq",
    20  "iswatchonly": false,
    21  "isscript": false,
    22  "iswitness": true,
    23  "witness_version": 0,
    24  "witness_program": "f7904103854f5c4440b0e3089ea3aa4d7732db62",
    25  "pubkey": "028f681e25caadc71f45e7811775f4e73b5b27ce7bf335888d514a9fc3999c55c1",
    26  "ischange": false,
    27  "timestamp": 1588011190,
    28  "hdkeypath": "m/0'/0'/0'",
    29  "hdseedid": "50a32a27b26929927079cef87b19a7ffb673871b",
    30  "hdmasterfingerprint": "6352002d",
    31  "labels": [
    32    "testing-signet"
    33  ]
    34}
    

    Obtain 10 signet coins at https://signet.bc-2.jp/

    0Payment of 10.00000000 BTC sent with txid 1fb49b9b484e42f5f19a1e5491b1760f949c303a297a63b68e41e9e9f379b3d5
    

    Verify reception

     0./bitcoin-cli -datadir=signet gettransaction 1fb49b9b484e42f5f19a1e5491b1760f949c303a297a63b68e41e9e9f379b3d5
     1{
     2  "amount": 10.00000000,
     3  "confirmations": 0,
     4  "trusted": false,
     5  "txid": "1fb49b9b484e42f5f19a1e5491b1760f949c303a297a63b68e41e9e9f379b3d5",
     6  "walletconflicts": [
     7  ],
     8  "time": 1588011612,
     9  "timereceived": 1588011612,
    10  "bip125-replaceable": "no",
    11  "details": [
    12    {
    13      "address": "sb1q77gyzqu9fawygs9suvyfaga2f4mn9kmz2yudk0",
    14      "category": "receive",
    15      "amount": 10.00000000,
    16      "label": "testing-signet",
    17      "vout": 0
    18    }
    19  ],
    20  "hex": "0200000000010154c0f4e926d77105e30311dd126f3a06aed63f7aa722702dd0c606e2f05a8db80100000000feffffff0200ca9a3b00000000160014f7904103854f5c4440b0e3089ea3aa4d7732db62fc33c6b8000000001600149218f528e0ebd953fdc72a7f42150facbe545ee1024730440220422cd8b64cdd49a87636ada6d41bfa761b592f99edc7d21081ba2b54e4d0672502207156fba2cec09d990a243c6925f4bca53e123730d13076d3e68cc0f96795e645012102a53a66b296489e8cd996c216d2c90e682187cce883f55d06c188daf5e8779816101f0000"
    21}
    22
    23./bitcoin-cli -datadir=signet getbalances
    24{
    25  "mine": {
    26    "trusted": 0.00000000,
    27    "untrusted_pending": 10.00000000,
    28    "immature": 0.00000000
    29  }
    30}
    

    …and after a few minutes…

     0./bitcoin-cli -datadir=signet getbalances
     1{
     2  "mine": {
     3    "trusted": 10.00000000,
     4    "untrusted_pending": 0.00000000,
     5    "immature": 0.00000000
     6  }
     7}
     8
     9./bitcoin-cli -datadir=signet -getinfo
    10{
    11  "version": 209900,
    12  "blocks": 8055,
    13  "headers": 8055,
    14  "verificationprogress": 1.544713757023421e-05,
    15  "timeoffset": 0,
    16  "connections": 3,
    17  "proxy": "",
    18  "difficulty": 0.0008245888870457042,
    19  "chain": "signet",
    20  "keypoolsize": 999,
    21  "paytxfee": 0.00000000,
    22  "balance": 10.00000000,
    23  "relayfee": 0.00001000,
    24  "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
    25}
    26
    27./bitcoin-cli -datadir=signet stop
    
  45. in src/primitives/block.h:137 in 76047d0c77 outdated
    132+     * Returns -1 if no witness commitment was found.
    133+     */
    134+    template<typename T> static inline int GetWitnessCommitmentIndex(const T& coinbase) {
    135+        for (int64_t o = coinbase.vout.size() - 1; o > -1; --o) {
    136+            auto vospk = coinbase.vout[o].scriptPubKey;
    137+            if (vospk.size() >= 38 && vospk[0] == OP_RETURN && vospk[1] == 0x24 && vospk[2] == 0xaa && vospk[3] == 0x21 && vospk[4] == 0xa9 && vospk[5] == 0xed) {
    


    jonatack commented at 1:46 pm on April 28, 2020:
    in 63b5d7a, if #18780 is merged, can replace 38 with MINIMUM_WITNESS_COMMITMENT

    fjahr commented at 2:28 pm on April 29, 2020:
    Agree, I was even going to suggest pulling the whole SigNet header into a named constant and then work with that.

    kallewoof commented at 4:23 pm on April 29, 2020:

    I was wondering about whether anyone would knee-jerk if we #include <script/script.h>’d, but then I realized this is already happening (evidenced by the fact OP_RETURN does not give a compile error here) via primitives/transaction.h, so it should be fine.

    Switching to this, thanks!


    jonatack commented at 4:45 pm on April 29, 2020:
    Maybe have a look at #18780 that looks like it’s about to be merged to see if you want to pull in some of the changes.

    kallewoof commented at 5:10 pm on April 29, 2020:

    Will check out #18780 for sure, thanks.

    Also, @fjahr this is not signet stuff, this is segwit stuff (the 38 + OP_RET + etc). It’s the witness commitment header. Or are you referring to something else when you say “the whole SigNet header into a named constant”?

  46. in src/primitives/block.h:126 in 76047d0c77 outdated
    121+     *
    122+     * Returns -1 if no witness commitment was found.
    123+     */
    124+    inline int GetWitnessCommitmentIndex() const
    125+    {
    126+        return vtx.empty() ? -1 : GetWitnessCommitmentIndex(*vtx.at(0));
    


    jonatack commented at 1:56 pm on April 28, 2020:
    in 63b5d7a3a7e6081e perhaps replace the four -1 commitment index values in block.h/.cpp with a static constant whose name could also make their meaning explicit

    kallewoof commented at 4:28 pm on April 29, 2020:
    Makes sense; also switching the couple of places in validation.cpp where -1 is used.
  47. in src/chainparams.cpp:268 in de4ef268a3 outdated
    271-        }
    272-        bin = ParseHex(args.GetArgs("-signet_blockscript")[0]);
    273-        if (args.IsArgSet("-signet_seednode")) {
    274-            vSeeds = gArgs.GetArgs("-signet_seednode");
    275-        }
    276+            LogPrintf("Using default signet network\n");
    


    fjahr commented at 2:52 pm on April 28, 2020:
    nit: s/signet/SigNet/

    kallewoof commented at 5:02 pm on April 29, 2020:
    I don’t think we call any of the other nets RegTest or TestNet, so signet seems OK here. I did a regexp search for "([^"]*)testnet and all instances I found used lowercase except for start-of-sentence-uppercasing.

    jonatack commented at 5:08 pm on April 29, 2020:
    :+1: on signet

    fjahr commented at 5:12 pm on April 29, 2020:
    Ok, then it’s just inconsistent because I saw SigNet as well in comments and log messages.

    kallewoof commented at 8:44 am on May 1, 2020:
    Yep, sorry about that. Fixed.
  48. in src/chainparams.cpp:276 in 76047d0c77 outdated
    271+            vSeeds.push_back("ntv3mtqw5wt63red.onion:38333");
    272+        } else {
    273+            if (args.GetArgs("-signet_blockscript").size() != 1) {
    274+                throw std::runtime_error(strprintf("%s: -signet_blockscript cannot be multiple values.", __func__));
    275+            }
    276+            bin = ParseHex(args.GetArgs("-signet_blockscript")[0]);
    


    jonatack commented at 8:22 pm on April 28, 2020:

    in d55bcb17844e6e68 perhaps cache the args in a localvar

     0-            if (args.GetArgs("-signet_blockscript").size() != 1) {
     1+            const auto sbs = args.GetArgs("-signet_blockscript");
     2+            if (sbs.size() != 1) {
     3                 throw std::runtime_error(strprintf("%s: -signet_blockscript cannot be multiple values.", __func__));
     4             }
     5-            bin = ParseHex(args.GetArgs("-signet_blockscript")[0]);
     6+            bin = ParseHex(sbs[0]);
     7             if (args.IsArgSet("-signet_seednode")) {
     8                 vSeeds = gArgs.GetArgs("-signet_seednode");
     9             }
    10 
    11-            LogPrintf("SigNet with block script %s\n", gArgs.GetArgs("-signet_blockscript")[0]);
    12+            LogPrintf("SigNet with block script %s\n", sbs[0]);
    

    kallewoof commented at 4:31 pm on April 29, 2020:
    Sounds good!
  49. in src/chainparams.cpp:302 in 76047d0c77 outdated
    294+        consensus.nPowTargetSpacing = 10 * 60;
    295+        consensus.fPowAllowMinDifficultyBlocks = false;
    296+        consensus.fPowNoRetargeting = false;
    297+        consensus.nRuleChangeActivationThreshold = 1916;
    298+        consensus.nMinerConfirmationWindow = 2016;
    299+        consensus.powLimit = uint256S("00002adc28cf53b63c82faa55d83e40ac63b5f100aa5d8df62a429192f9e8ce5");
    


    jonatack commented at 8:36 pm on April 28, 2020:
    In d55bcb1, how did you arrive at this powLimit value?

    kallewoof commented at 4:33 pm on April 29, 2020:
    I grinded a block for a short while and picked the lowest resulting value. It is a bit more random than it needs to be, but doesn’t seem harmful so I kept it as is.
  50. in src/chainparams.cpp:336 in 76047d0c77 outdated
    323+
    324+        bech32_hrp = "sb" + args.GetArg("-signet_hrp", "");
    325+
    326+        fDefaultConsistencyChecks = false;
    327+        fRequireStandard = true;
    328+        m_is_test_chain = true;
    


    jonatack commented at 8:45 pm on April 28, 2020:

    d55bcb1 do we need any of these additional params to be set in SigNetParams? (values tba). Some of them may have been added since this PR was made.

     0+        // The best chain should have at least this much work.
     1+        consensus.nMinimumChainWork = uint256S("0x0000000000000000000000000000000000000000000001495c1d5a01e2af8a23");
     2+
     3+        // By default assume that the signatures in ancestors of this block are valid.
     4+        consensus.defaultAssumeValid = uint256S("0x000000000000056c49030c174179b52a928c870e6e8a822c75973b7970cfbd01"); // 1692000
     5 
     6         nDefaultPort = 38333;
     7         nPruneAfterHeight = 1000;
     8+        m_assumed_blockchain_size = 40;
     9+        m_assumed_chain_state_size = 2;
    10 
    11         fDefaultConsistencyChecks = false;
    12         fRequireStandard = true;
    13         m_is_test_chain = true;
    14+        m_is_mockable_chain = true;
    

    kallewoof commented at 4:34 pm on April 29, 2020:
    I didn’t notice these additions, thanks. Adding. Edit: well, the first ones are from before; I was simply not including a minimum chain work or default assume valid.
  51. in src/chainparamsbase.cpp:29 in 76047d0c77 outdated
    23@@ -23,6 +24,10 @@ void SetupChainParamsBaseOptions()
    24     gArgs.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    25     gArgs.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    26     gArgs.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    27+    gArgs.AddArg("-signet", "Use the signet chain. Note that the network is defined by the signet_blockscript parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    28+    gArgs.AddArg("-signet_blockscript", "Blocks must satisfy the given script to be considered valid (only for signet networks)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
    29+    gArgs.AddArg("-signet_hrp", "Human readable part of bech32 address (suffixed by \"sb\"; default = \"\")", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    30+    gArgs.AddArg("-signet_seednode", "Specify a seed node for the signet network (may be used multiple times to specify multiple seed nodes)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
    


    jonatack commented at 10:54 pm on April 28, 2020:
    d55bcb1 I like the readability of snakecased config options, but is the current convention that they be without separators? e.g. -signetblockscript, -signethrp, -signetseednode

    fjahr commented at 3:14 pm on April 29, 2020:
    s/signet/SigNet/

    kallewoof commented at 4:39 pm on April 29, 2020:
    I haven’t heard of such a convention for options. I’ll keep this until someone else yells at me, as it’s spread around a bit. (Will change if it’s important though!)

    jonatack commented at 5:17 pm on May 5, 2020:

    What is the difference between the -signet_seednode chainparams option, and the -seednode connection option in init.cpp?

    Perhaps (pulling in some ideas from the -seednode help):

    0-    gArgs.AddArg("-signet_seednode", "Specify a seed node for the signet network (may be used multiple times to specify multiple seed nodes)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
    1+    gArgs.AddArg("-signet_seednode=<ip>", "Specify a seed node for the signet network (may be specified multiple times to connect to multiple seed nodes)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
    

    kallewoof commented at 4:20 am on May 6, 2020:
    I’m not entirely sure; it seems the -seednode flag is used to set up CConnman options in init.cpp, but I can’t see that using the chain parameter stuff, so… yeah, not sure.
  52. in src/chainparamsbase.cpp:29 in 76047d0c77 outdated
    23@@ -23,6 +24,10 @@ void SetupChainParamsBaseOptions()
    24     gArgs.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    25     gArgs.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    26     gArgs.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    27+    gArgs.AddArg("-signet", "Use the signet chain. Note that the network is defined by the signet_blockscript parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    28+    gArgs.AddArg("-signet_blockscript", "Blocks must satisfy the given script to be considered valid (only for signet networks)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
    29+    gArgs.AddArg("-signet_hrp", "Human readable part of bech32 address (suffixed by \"sb\"; default = \"\")", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    


    jonatack commented at 10:57 pm on April 28, 2020:
    d55bcb1 nit: s/Human readable/Human-readable/

    kallewoof commented at 4:38 pm on April 29, 2020:
    Both seem to be acceptable. E.g. https://en.wikipedia.org/wiki/Human-readable_medium has as an example the sentence Human readable protocols greatly reduce the cost of debugging.[1].

    MarcoFalke commented at 10:44 am on May 5, 2020:
    Mind to explain the use case for signet_hrp being a run-time option?

    jonatack commented at 5:35 pm on May 5, 2020:

    Mind to explain the use case for signet_hrp being a run-time option?

    Agree; I don’t know how this is to be used.


    kallewoof commented at 4:14 am on May 6, 2020:

    This became useful when I was running a taproot signet there for awhile, as it allowed correctly configured nodes to distinguish between addresses for the two networks.

    I think we are leaning more and more towards having “one” signet most of the time, but having this feature still seems like a good idea. I don’t have a strong opinion, and can drop this out for later consideration if you guys feel uncomfortable about it.


    MarcoFalke commented at 10:55 am on May 6, 2020:

    it allowed correctly configured nodes …

    That assumes correct configuration. In converse, incorrect configuration will reject addresses of the own network. Leaving this option out will configure all nodes correctly and I don’t see a risk where mixing up addresses could lead to an issue. It could even make sense to call it tb, see here #12314


    jonatack commented at 1:54 pm on May 6, 2020:

    s/suffixed/prefixed/?

    Maybe add a usage example.


    kallewoof commented at 6:26 am on May 7, 2020:
    I don’t know about calling it tb, but I’ve seen enough friction to warrant removing this option in the first iteration/PR, so I’m removing this parameter for now.
  53. in src/chainparamsbase.cpp:47 in 76047d0c77 outdated
    45@@ -41,8 +46,9 @@ std::unique_ptr<CBaseChainParams> CreateBaseChainParams(const std::string& chain
    46         return MakeUnique<CBaseChainParams>("testnet3", 18332);
    47     else if (chain == CBaseChainParams::REGTEST)
    48         return MakeUnique<CBaseChainParams>("regtest", 18443);
    49-    else
    50-        throw std::runtime_error(strprintf("%s: Unknown chain %s.", __func__, chain));
    51+    else if (chain == CBaseChainParams::SIGNET)
    52+        return MakeUnique<CBaseChainParams>("signet", 38332);
    


    jonatack commented at 11:05 pm on April 28, 2020:

    In d55bcb1, maintain the same sort (main/testnet/signet/regtest) here as in chainparamsbase.h?

    0-    else if (chain == CBaseChainParams::REGTEST)
    1-        return MakeUnique<CBaseChainParams>("regtest", 18443);
    2     else if (chain == CBaseChainParams::SIGNET)
    3         return MakeUnique<CBaseChainParams>("signet", 38332);
    4+    else if (chain == CBaseChainParams::REGTEST)
    5+        return MakeUnique<CBaseChainParams>("regtest", 18443);
    

    Also, should we keep the else before the throw?


    kallewoof commented at 4:42 pm on April 29, 2020:
    Swapped; I don’t see the point keeping the else as every if case returns, and it seems more clear to be explicit, but I can see the argument for minimal diffs…
  54. in src/chainparams.cpp:491 in 76047d0c77 outdated
    476@@ -396,6 +477,9 @@ std::unique_ptr<const CChainParams> CreateChainParams(const std::string& chain)
    477         return std::unique_ptr<CChainParams>(new CTestNetParams());
    478     else if (chain == CBaseChainParams::REGTEST)
    479         return std::unique_ptr<CChainParams>(new CRegTestParams(gArgs));
    480+    else if (chain == CBaseChainParams::SIGNET) {
    


    jonatack commented at 12:13 pm on April 29, 2020:
    d55bcb1 perhaps maintain the same sort (main/testnet/signet/regtest) here (and in general wherever the four chains are enumerated)?
  55. in src/validation.cpp:1167 in 76047d0c77 outdated
    1151@@ -1151,6 +1152,11 @@ bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::P
    1152     if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
    1153         return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
    1154 
    1155+    if (consensusParams.signet_blocks && block.GetHash() != consensusParams.hashGenesisBlock && !CheckBlockSolution(block, consensusParams)) {
    


    jonatack commented at 1:06 pm on April 29, 2020:
    db73201 istm a comment here and at line 3300 below would be helpful (the neighbouring code is commented as well)
  56. in src/primitives/block.cpp:62 in 76047d0c77 outdated
    57+    CScript result;
    58+    std::vector<uint8_t> pushdata;
    59+    auto script = mtx.vout[cidx].scriptPubKey;
    60+    opcodetype opcode;
    61+    CScript::const_iterator pc = script.begin();
    62+    result.push_back(*pc++);
    


    jonatack commented at 1:29 pm on April 29, 2020:

    63b5d7a3a7e6081e3b42a78ad1f nit: would it be better to construct rather than copy?

    0-    result.push_back(*pc++);
    1+    result.emplace_back(*pc++);
    

    kallewoof commented at 4:46 pm on April 29, 2020:
    Switched to emplace_back
  57. in src/test/util_tests.cpp:1109 in 76047d0c77 outdated
    1124@@ -1125,7 +1125,7 @@ BOOST_FIXTURE_TEST_CASE(util_ChainMerge, ChainMergeTestingSetup)
    1125     // Results file is formatted like:
    1126     //
    1127     //   <input> || <output>
    1128-    BOOST_CHECK_EQUAL(out_sha_hex, "f0b3a3c29869edc765d579c928f7f1690a71fbb673b49ccf39cbc4de18156a0d");
    1129+    BOOST_CHECK_EQUAL(out_sha_hex, "4645298a210e40fd8dfacc2b470be96a186f0ff2ebab328953cfe1ce866e55cf");
    


    fjahr commented at 1:52 pm on April 29, 2020:
    Not knowing this test, I was confused why this is needed until I dumped the file. So a little better explanation would be nice in the commit message. Suggestion: “Even though the test is not changed, error messages for missing/invalid parameter combinations now include -signet. This is why the hash changes.”

    jonatack commented at 4:33 pm on April 29, 2020:
    I was wondering this, too.

    kallewoof commented at 5:03 pm on April 29, 2020:
    Apologies; I don’t actually expect this commit to make it through in the end. I should mark it as such. I believe this is something @laanwj wants to double check and commit when appropriate, but I may be off here.

    kallewoof commented at 3:43 am on April 30, 2020:

    Update: I looked closer into this (my initial impression was that this was a maintainer thing, but it actually looks like the contributors are supposed to deal with it), and have updated the commit message to explain why the hash changed.

    Also, a recent addition to tests resulted in another hash changing; these were combined in one commit.

  58. jonatack commented at 2:02 pm on April 29, 2020: member

    ACK 76047d0c77f0ade5b2e16f5d980c4d4b1709a13b with a few questions and minor comments below.

    Code review, gcc/clang/fuzz builds, unit/functional tests green. Manually tested bitcoind with signet per my last comment above, and it ran successfully for a day. Also tested the GUI (./src/qt/bitcoin-qt -datadir=signet) and ran various rpc commands in the console.

    Screenshot from 2020-04-29 15-52-26

  59. in src/primitives/block.h:158 in 63b5d7a3a7 outdated
    140+        }
    141+        return -1;
    142+    }
    143+
    144+    /**
    145+     * Attempt to get the data for the section with the given header in the witness commitment of this block.
    


    fjahr commented at 2:21 pm on April 29, 2020:
    I think there could be an additional comment for these CommitmentSection functions that they are specific for SigNet

    kallewoof commented at 5:05 pm on April 29, 2020:
    Technically they’re not; this code simply adds support for a feature that segwit always supported, which is the extension of the witness commitment to add arbitrary/additional data. This adds a framework to find/modify such extensions, and includes a header for each one, so they are distinguishable.
  60. in src/script/interpreter.h:169 in d50d6acb9f outdated
    160@@ -161,6 +161,17 @@ class BaseSignatureChecker
    161     virtual ~BaseSignatureChecker() {}
    162 };
    163 
    164+/** A low level signature checker.  Note that the caller must verify that the hash passed in relates to a known message (unlike for the other signature checkers). */
    


    fjahr commented at 2:34 pm on April 29, 2020:

    super nit

    0/** A low level signature checker. Note that it does not verify that the hash passed in relates to a known message (unlike for the other signature checkers). */
    

    jonatack commented at 4:32 pm on April 29, 2020:
    ISTM the first version is correct?

    fjahr commented at 4:36 pm on April 29, 2020:

    ISTM the first version is correct?

    That’s why it’s a nit.


    fjahr commented at 4:38 pm on April 29, 2020:
    There is a double space and it assumes that the caller has to do something which is why I like my version a bit better.

    jonatack commented at 4:38 pm on April 29, 2020:
    Yes, I mean that I think the first version is better.

    fjahr commented at 4:42 pm on April 29, 2020:

    Yes, I mean that I think the first version is better.

    How can you be sure that the caller always has to do this whenever this function is used?


    jonatack commented at 4:49 pm on April 29, 2020:

    There is a double space and it assumes that the caller has to do something which is why I like my version a bit better.

    Double space is grammatically correct and the warning to the caller seems preferable IMO.


    fjahr commented at 4:53 pm on April 29, 2020:
    That’s my point: “must” is not a warning, it’s a requirement.

    jonatack commented at 5:10 pm on April 29, 2020:
    I read it as “the caller is responsible for verifying” which is more defensively paranoid, in a good way, I think, than “it does not verify”.

    kallewoof commented at 5:12 pm on April 29, 2020:
    The current version seems a tiny bit more clear on the fact that if you use this thing, you (the caller) have to check this stuff yourself, so keeping as is.
  61. in src/chainparams.cpp:267 in d55bcb1784 outdated
    262+
    263+        std::vector<uint8_t> bin;
    264+        vSeeds.clear();
    265+
    266+        if (!args.IsArgSet("-signet_blockscript")) {
    267+            throw std::runtime_error(strprintf("%s: -signet_blockscript is mandatory for signet networks", __func__));
    


    fjahr commented at 3:11 pm on April 29, 2020:
    nit s/signet/SigNet/
  62. in src/chainparamsbase.cpp:27 in d55bcb1784 outdated
    23@@ -23,6 +24,10 @@ void SetupChainParamsBaseOptions()
    24     gArgs.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    25     gArgs.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    26     gArgs.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    27+    gArgs.AddArg("-signet", "Use the signet chain. Note that the network is defined by the signet_blockscript parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    


    fjahr commented at 3:13 pm on April 29, 2020:
    s/signet/SigNet/
  63. jonatack commented at 3:54 pm on April 29, 2020: member
    Made a few signet transactions using the GUI. Works well.
  64. in src/chainparams.cpp:284 in d55bcb1784 outdated
    269+        if (args.GetArgs("-signet_blockscript").size() != 1) {
    270+            throw std::runtime_error(strprintf("%s: -signet_blockscript cannot be multiple values.", __func__));
    271+        }
    272+        bin = ParseHex(args.GetArgs("-signet_blockscript")[0]);
    273+        if (args.IsArgSet("-signet_seednode")) {
    274+            vSeeds = gArgs.GetArgs("-signet_seednode");
    


    fjahr commented at 4:22 pm on April 29, 2020:
    nit: I guess starting a custom signet without a seed node only makes sense if the user is running the seed node themselves. It might be worth a warning or a log message in case people wonder why nothing is happening if they forget this.

    kallewoof commented at 5:08 pm on April 29, 2020:
    I think it will be fairly common to start a node and then manually addnode= peers to build a network real quick. Having a seed node is only really meant for super permanent signets (like the default one) where you want people to just be able to start and go. I’m not sure how this will all pan out, but that’s how I envision it right now.

    jonatack commented at 1:48 pm on May 6, 2020:
    It may still be good to change the output if -seednode is passed and echo it rather than only “Using default signet network”

    ajtowns commented at 11:14 pm on August 27, 2020:
    nit: args not gArgs
  65. fjahr commented at 4:22 pm on April 29, 2020: member

    Concept ACK

    I will do another manual test with the latest code before finishing the code review. In the meantime, I have some (mostly nit) comments and suggestions.

    First a conceptual question: I might be missing something and this might have been discussed previously but as a participant in the network (not the signer) what prevents me from taking the last block, mining a higher POW for it and then sending it out to replace the original block from the signer? If I understand it correctly, this is possible but attacks with this are probably not feasible because the attacker only has the block nonce to play with, not the extended nonce and they can also not change the other content of the block. For a moment I thought this might be a problem but since an attack with this would probably require many blocks, this is not a concern I think. I am just generally interested if I got this part right.

    More general comments (did not make sense as inline comments):

    • You might plan to add them later, but I thought adding SIGNET to tests in src/test/util_tests.cpp:870/871 and src/test/key_io_tests.cpp:139 might make sense in this pull already
    • The first commit could be split into two: the move only part and the witness commitment section, for easier review
  66. kallewoof force-pushed on Apr 29, 2020
  67. kallewoof commented at 5:13 pm on April 29, 2020: member
    Thanks a lot @jonatack and @fjahr for your feedback. I’ve addressed all of your comments (let me know if I ignored something), so please re-review!
  68. fjahr commented at 5:27 pm on April 29, 2020: member

    First a conceptual question: I might be missing something and this might have been discussed previously but as a participant in the network (not the signer) what prevents me from taking the last block, mining a higher POW for it and then sending it out to replace the original block from the signer? If I understand it correctly, this is possible but attacks with this are probably not feasible because the attacker only has the block nonce to play with, not the extended nonce and they can also not change the other content of the block. For a moment I thought this might be a problem but since an attack with this would probably require many blocks, this is not a concern I think. I am just generally interested if I got this part right.

    More general comments (did not make sense as inline comments):

    • You might plan to add them later, but I thought adding SIGNET to tests in src/test/util_tests.cpp:870/871 and src/test/key_io_tests.cpp:139 might make sense in this pull already
    • The first commit could be split into two: the move only part and the witness commitment section, for easier review @kallewoof I think you missed my comments in the review message? ^ Thanks!
  69. in src/chainparams.cpp:256 in 3b4e8d78f6 outdated
    251@@ -249,13 +252,95 @@ class CTestNetParams : public CChainParams {
    252     }
    253 };
    254 
    255+/**
    256+ * SigNet
    


    fjahr commented at 7:49 pm on April 29, 2020:
    s/SigNet/Signet/

    kallewoof commented at 3:34 am on April 30, 2020:
    A-ha! Thanks, my consistency is admirable… Fixed.
  70. in src/chainparams.cpp:282 in 3b4e8d78f6 outdated
    277+            bin = ParseHex(blockscript[0]);
    278+            if (args.IsArgSet("-signet_seednode")) {
    279+                vSeeds = gArgs.GetArgs("-signet_seednode");
    280+            }
    281+
    282+            LogPrintf("SigNet with block script %s\n", blockscript[0]);
    


    fjahr commented at 7:50 pm on April 29, 2020:
    s/SigNet/Signet/
  71. kallewoof force-pushed on Apr 30, 2020
  72. kallewoof force-pushed on Apr 30, 2020
  73. kallewoof commented at 3:33 am on April 30, 2020: member

    @fjahr Indeed, sorry about that!

    First a conceptual question: I might be missing something and this might have been discussed previously but as a participant in the network (not the signer) what prevents me from taking the last block, mining a higher POW for it and then sending it out to replace the original block from the signer? If I understand it correctly, this is possible but attacks with this are probably not feasible because the attacker only has the block nonce to play with, not the extended nonce and they can also not change the other content of the block. For a moment I thought this might be a problem but since an attack with this would probably require many blocks, this is not a concern I think. I am just generally interested if I got this part right.

    Yep, you did. There’s nothing preventing you from doing this, and as Signet itself comes with built in double spend modes, this is going to be a fairly common occurrence for certain networks. I don’t think it’s a big problem, though.

    • You might plan to add them later, but I thought adding SIGNET to tests in src/test/util_tests.cpp:870/871 and src/test/key_io_tests.cpp:139 might make sense in this pull already

    Makes sense, thanks!

    • The first commit could be split into two: the move only part and the witness commitment section, for easier review

    You’re right, that makes more sense. Done.

  74. kallewoof force-pushed on Apr 30, 2020
  75. kallewoof force-pushed on Apr 30, 2020
  76. kallewoof force-pushed on Apr 30, 2020
  77. DrahtBot added the label Needs rebase on Apr 30, 2020
  78. fjahr commented at 2:57 pm on April 30, 2020: member

    Needs another rebase, but ACK 27619339eb4dc348a32aa68b517be5b49df3d6a3. Reviewed code including latest changes and synced a node and tested sending and receiving txs and some other basic functionalities, mostly info calls.

    One thing I noticed is that the progress during sync currently doesn’t work (shows 1.0 from the start) because there is no chainTxData for Signet, I guess that is something that gets added by maintainers? EDIT: Yes it is. https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-branch-off

  79. brakmic commented at 6:11 pm on April 30, 2020: contributor

    ACK 27619339eb4dc348a32aa68b517be5b49df3d6a3

    Built, run and tested on macOS Catalina 10.15.4

    Unit tests executed successfully.

    Done manual tests with bitcoind and bitcoin.conf that contained signet=1 flag.

    Examples:

    • Query daemon info

    ./bitcoin-cli -datadir=signet -getinfo

     0{
     1  "version": 209900,
     2  "blocks": 2875,
     3  "headers": 8376,
     4  "verificationprogress": 7.824597019403681e-230,
     5  "timeoffset": 0,
     6  "connections": 1,
     7  "proxy": "",
     8  "difficulty": 0.0007813477653418144,
     9  "chain": "signet",
    10  "keypoolsize": 1000,
    11  "paytxfee": 0.00000000,
    12  "balance": 0.00000000,
    13  "relayfee": 0.00001000,
    14  "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
    15}
    
    • Created a signet address and received 10 signet BTCs:
    0./bitcoin-cli -datadir=signet getnewaddress
    1sb1q3tsk7vvs3guyg6cacxd8celepqjqupes4ura2t
    
    0./bitcoin-cli -datadir=signet setlabel sb1q3tsk7vvs3guyg6cacxd8celepqjqupes4ura2t brakmic-signet-address
    
     0./bitcoin-cli -datadir=signet getaddressinfo sb1q3tsk7vvs3guyg6cacxd8celepqjqupes4ura2t
     1{
     2  "address": "sb1q3tsk7vvs3guyg6cacxd8celepqjqupes4ura2t",                     
     3  "scriptPubKey": "00148ae16f31908a38446b1dc19a7c67f908240e0730",
     4  "ismine": true,
     5  "solvable": true,
     6  "desc": "wpkh([39028b98/0'/0'/0']03164fe243eda4c563657cc10b0c6f420fc92dde092f67b5e03f7d7ddfe20c9a6f)#l9u634et",
     7  "iswatchonly": false,
     8  "isscript": false,
     9  "iswitness": true,
    10  "witness_version": 0,
    11  "witness_program": "8ae16f31908a38446b1dc19a7c67f908240e0730",
    12  "pubkey": "03164fe243eda4c563657cc10b0c6f420fc92dde092f67b5e03f7d7ddfe20c9a6f",
    13  "ischange": false,
    14  "timestamp": 1588269529,
    15  "hdkeypath": "m/0'/0'/0'",
    16  "hdseedid": "0ba40a67510bf4857fd667d106e3f67c99257524",
    17  "hdmasterfingerprint": "39028b98",
    18  "labels": [
    19    "brakmic-signet-address"
    20  ]
    21}
    
     0./bitcoin-cli -datadir=signet gettransaction 2588fc33f81a53994648773591d4a6e127535c74b6340dee19e5608d9547a9ce
     1{
     2  "amount": 10.00000000,
     3  "confirmations": 0,
     4  "trusted": false,
     5  "txid": "2588fc33f81a53994648773591d4a6e127535c74b6340dee19e5608d9547a9ce",
     6  "walletconflicts": [
     7  ],
     8  "time": 1588269697,
     9  "timereceived": 1588269697,
    10  "bip125-replaceable": "no",
    11  "details": [
    12    {
    13      "address": "sb1q3tsk7vvs3guyg6cacxd8celepqjqupes4ura2t",
    14      "category": "receive",
    15      "amount": 10.00000000,
    16      "label": "brakmic-signet-address",
    17      "vout": 0
    18    }
    19  ],
    20  "hex": "0200000000010159fedd1084d29c3c4615254d1b3b5786c4f01c1a497e39762f40be3ec05b7b8d0000000000feffffff0200ca9a3b000000001600148ae16f31908a38446b1dc19a7c67f908240e073076c5f50500000000160014b342968beebfe1b0d17fe385e167567ad07811e902473044022053cc1d2a66739fb4b2ad6f284dff56cae186d7c9d464f4a1448ffcda4df95828022030f49b1c7d32544ce247d0a828254aa81b6ed315a00f37d225cee4585eeeb1f5012103c7f54159ca13bb20f515813df57de41cedae3c71dafc4b4150ab5b06ed72a527b8200000"
    21}
    
    • My wallet balances (not confirmed)
    0./bitcoin-cli -datadir=signet getbalances                   
    1{
    2  "mine": {
    3    "trusted": 0.00000000,
    4    "untrusted_pending": 10.00000000,
    5    "immature": 0.00000000
    6  }
    7}
    
    • Same balances after a few minutes
    0./bitcoin-cli -datadir=signet getbalances
    1{
    2  "mine": {
    3    "trusted": 10.00000000,
    4    "untrusted_pending": 0.00000000,
    5    "immature": 0.00000000
    6  }
    7}
    
  80. kallewoof force-pushed on Apr 30, 2020
  81. DrahtBot removed the label Needs rebase on Apr 30, 2020
  82. kallewoof commented at 8:43 am on May 1, 2020: member

    @fjahr

    Thanks for the re-review; rebased!

    One thing I noticed is that the progress during sync currently doesn’t work (shows 1.0 from the start) because there is no chainTxData for Signet, I guess that is something that gets added by maintainers? EDIT: Yes it is. https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md#before-branch-off

    Yeah, I think there’s discussion to actually only display this for specific networks (mainnet, and testnet I guess). Since you can spin up custom signets, it seems not worthwhile to add checkpoints and such, but it’s up to the contributors whether they do or not. @brakmic Thanks a lot for the detailed review. Would love a re-ACK now that it’s been rebased :)

  83. brakmic commented at 8:54 am on May 1, 2020: contributor

    @brakmic Thanks a lot for the detailed review. Would love a re-ACK now that it’s been rebased :)

    You’re welcome!

    I’ve recompiled your latest code as soon as it was available…on my Raspberry Pi4 :)

    0uname -a
    1Linux lnd 4.19.105-v8+ [#1296](/bitcoin-bitcoin/1296/) SMP PREEMPT Thu Feb 20 16:34:11 GMT 2020 aarch64 GNU/Linux
    

    Just wanted to know how it works there. And I am keeping it online.

    0./bitcoin-cli -datadir=signet uptime
    139362
    
     0./bitcoin-cli -datadir=signet getblockchaininfo
     1{
     2  "chain": "signet",
     3  "blocks": 8464,
     4  "headers": 8464,
     5  "bestblockhash": "000002620188a0abe05b324d32ec943b71a593a6e2a577df6490af59079801c9",
     6  "difficulty": 0.0008142809685956302,
     7  "mediantime": 1588319843,
     8  "verificationprogress": 7.753189433667663e-09,
     9  "initialblockdownload": false,
    10  "chainwork": "0000000000000000000000000000000000000000000000000000000564eb9c63",
    11  "size_on_disk": 3244862,
    12  "pruned": false,
    

    Awesome work, @kallewoof !

    ACK 483fbce2b87de171112a0f304095797a6b5d3d08

  84. fjahr commented at 1:15 pm on May 1, 2020: member

    re-ACK 483fbce2b87de171112a0f304095797a6b5d3d08

    Only changes since last review were rebase and movement + usage of MINIMUM_WITNESS_COMMITMENT constant.

    Yeah, I think there’s discussion to actually only display this for specific networks (mainnet, and testnet I guess). Since you can spin up custom signets, it seems not worthwhile to add checkpoints and such, but it’s up to the contributors whether they do or not.

    Makes sense, thanks!

  85. in src/primitives/block.h:141 in 8bbfde09fb outdated
    151+    /**
    152+     * Get the vout index of the segwit commitment in the given coinbase transaction.
    153+     *
    154+     * Returns -1 if no witness commitment was found.
    155+     */
    156+    template<typename T> static inline int GetWitnessCommitmentIndex(const T& coinbase) {
    


    MarcoFalke commented at 2:47 pm on May 1, 2020:

    I still don’t understand why this is moved #16411 (review)

    • GetWitnessCommitmentIndex is a member function, but doesn’t access any member variables. This is confusing and makes it hard to write clean code.
    • src/primitives only contains data structures, and no validation logic. If this becomes the new place for validation, we might as well move the rest of validation to this file. E.g. ConnectBlock to CBlock::Connect, etc. This clearly defeats the purpose of having different modules for primitives and validation.

    MarcoFalke commented at 2:49 pm on May 1, 2020:
    style-nit: member functions are already inline, same goes for template<>s. No need to specify it twice.

    kallewoof commented at 5:58 am on May 2, 2020:
    I hear you on the “only data structure” part… perhaps this belongs in validation.cpp in the end. It’s bloated and I didn’t wanna bloat it more with the new witness commitment section stuff, though.

    kallewoof commented at 6:03 am on May 2, 2020:
    On the other hand, it could be argued that this is in fact data structure stuff and not validation. I mean, there’s no validation done here, it simply locates and returns the index of the witness commitment, if any.

    MarcoFalke commented at 8:41 pm on May 2, 2020:

    GetWitnessCommitmentIndex has been added to validation in commit 8b49040854be2e26b66366aeae1cba4716f93d93. Generally we don’t move stuff around unless there is a reason to. Maybe @sipa can give more insights. Though, if no one else complains about this, I will close this conversation as resolved.

    However, making this a member function of CBlock, whereas it doesn’t read any of the member variables, is highly confusing and should be fixed.


    sipa commented at 8:43 pm on May 2, 2020:
    I strongly prefer keeping this in validation. Not every piece of code that uses transactions (e.g. wallets, P2P layer things, …) needs to depend on something that only matters to validation.

    kallewoof commented at 6:12 am on May 4, 2020:
    I’ve restored it to validation.cpp, and added a variant that takes a transaction rather than a block. The block-based one (original) is converted into a wrapper calling the variant.
  86. kallewoof force-pushed on May 2, 2020
  87. kallewoof commented at 6:06 am on May 2, 2020: member
    Removed inline; also replaced two Returns -1 if ... with Returns NO_WITNESS_COMMITMENT if ....
  88. kallewoof force-pushed on May 4, 2020
  89. kallewoof commented at 6:13 am on May 4, 2020: member

    After feedback, I’ve restored GetWitnessCommitmentIndex into validation.cpp.

    This also introduces the transaction-based variant which is used by the signet code.

    I.e.

    • the “move GWCI from validation into primitives/block” commit is gone
    • in its stead is a new “add tx based GWCI variants” commit
    • the remainder of the witness commitment section code has been moved into signet.h/cpp, and is now included in the add signet basic support commit
  90. kallewoof force-pushed on May 4, 2020
  91. kallewoof force-pushed on May 4, 2020
  92. kallewoof force-pushed on May 4, 2020
  93. kallewoof force-pushed on May 4, 2020
  94. kallewoof force-pushed on May 4, 2020
  95. kallewoof force-pushed on May 4, 2020
  96. kallewoof force-pushed on May 4, 2020
  97. in src/validation.h:335 in fd10dd5f00 outdated
    333@@ -332,6 +334,9 @@ bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& pa
    334 /** Compute at which vout of the block's coinbase transaction the witness commitment occurs, or -1 if not found */
    335 int GetWitnessCommitmentIndex(const CBlock& block);
    336 
    337+/** Compute at which vout of the given coinbase transaction the witness commitment occurs, or -1 if not found */
    


    kallewoof commented at 5:32 am on May 5, 2020:
    This comment should use the new constant, not -1. Will fix at next rebase.
  98. in src/signet.cpp:53 in 3e44470efa outdated
    48+        // find and delete signet signature
    49+        CMutableTransaction mtx(*block.vtx.at(0));
    50+        SetWitnessCommitmentSection(mtx, SIGNET_HEADER, std::vector<uint8_t>{});
    51+        leaves[0] = mtx.GetHash();
    52+    }
    53+    for (size_t s = 1; s < block.vtx.size(); s++) {
    


    jonatack commented at 3:22 pm on May 5, 2020:
    fd10dd5f and 6209c21 pico-nit: if you retouch, here and a couple other places in the changeset, ++s is preferred over s++ per developer-notes.md::L94
  99. in src/validation.cpp:3404 in 3e44470efa outdated
    3413+    for (size_t o = 0; o < tx.vout.size(); o++) {
    3414+        const CTxOut& vout = tx.vout[o];
    3415+        if (vout.scriptPubKey.size() >= MINIMUM_WITNESS_COMMITMENT &&
    3416+            vout.scriptPubKey[0] == OP_RETURN &&
    3417+            vout.scriptPubKey[1] == 0x24 &&
    3418+            vout.scriptPubKey[2] == 0xaa &&
    


    jonatack commented at 3:48 pm on May 5, 2020:

    fd10dd5f If you retouch, perhaps add comments to document the byte values:

    0-            vout.scriptPubKey[1] == 0x24 &&
    1-            vout.scriptPubKey[2] == 0xaa &&
    2+            vout.scriptPubKey[1] == 0x24 &&  // Push the following 36 bytes
    3+            vout.scriptPubKey[2] == 0xaa &&  // BIP141 SegWit commitment header (4 bytes: 0xaa21a9ed)
    

    kallewoof commented at 4:16 am on May 6, 2020:
    If you do ?w=1 suffix on the view all changes page [ https://github.com/bitcoin/bitcoin/pull/18267/files?w=1#diff-24efdb00bfbe56b140fb006b562cc70b ], you will note that I’m only indenting these lines, so I think I’ll skip touching them for now.

    jonatack commented at 12:45 pm on May 6, 2020:
    ok
  100. in src/chainparams.cpp:271 in 3e44470efa outdated
    266+        if (!args.IsArgSet("-signet_blockscript")) {
    267+            LogPrintf("Using default signet network\n");
    268+            bin = ParseHex("512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be43051ae");
    269+            vSeeds.push_back("178.128.221.177");
    270+            vSeeds.push_back("2a01:7c8:d005:390::5");
    271+            vSeeds.push_back("ntv3mtqw5wt63red.onion:38333");
    


    jonatack commented at 5:07 pm on May 5, 2020:
    c1144b86 These 3 lines can use emplace_back like the other vSeeds.emplace_back operations in this file.
  101. in src/chainparams.cpp:277 in 3e44470efa outdated
    275+                throw std::runtime_error(strprintf("%s: -signet_blockscript cannot be multiple values.", __func__));
    276+            }
    277+            bin = ParseHex(blockscript[0]);
    278+            if (args.IsArgSet("-signet_seednode")) {
    279+                vSeeds = gArgs.GetArgs("-signet_seednode");
    280+            }
    


    jonatack commented at 5:11 pm on May 5, 2020:
    c1144b86 -signet_seednode appears to only take effect if -signet_blockscript is also passed. If so, perhaps say so in the -signet_seednode help in chainparamsbase.cpp.

    kallewoof commented at 4:17 am on May 6, 2020:
    I don’t think that’s a good logic, actually. I’m going to change it to always take seeds, thanks.
  102. jonatack commented at 5:37 pm on May 5, 2020: member

    Light re-ACK 3e44470efaafa.

    Code review, build/tests, ran bitcoind and the GUI and focused a bit more on the -signet_blockscript config option:

    • Passed 2 values to -signet_blockscript
    0Error: SigNetParams: -signet_blockscript cannot be multiple values.
    
    • Passed in a bad -signet_blockscript
    0[0%]...ERROR: CheckBlockSolution: Errors in block (block solution invalid)
    12020-05-05T16:54:56Z ERROR: VerifyDB(): *** ReadBlockFromDisk failed at 9078, hash=0000045cba9402f733c6aeb5ab9dd6f68d898539b5bb0416c7260d09b80901de
    22020-05-05T16:54:56Z : Corrupted block database detected.
    3Please restart with -reindex or -reindex-chainstate to recover.
    4: Corrupted block database detected.
    5Please restart with -reindex or -reindex-chainstate to recover.
    

    A few comments follow if you re-touch.

  103. kallewoof force-pushed on May 6, 2020
  104. kallewoof commented at 4:28 am on May 6, 2020: member
    Addressed @jonatack nits.
  105. in src/chainparams.cpp:278 in 25be6f6c2d outdated
    273+        bin = ParseHex(blockscript[0]);
    274+        if (args.IsArgSet("-signet_seednode")) {
    275+            vSeeds = gArgs.GetArgs("-signet_seednode");
    276+        }
    277+
    278+        LogPrintf("Signet with block script %s\n", blockscript[0]);
    


    brakmic commented at 8:01 am on May 6, 2020:

    This line causes compilation errors, because of undeclared blockscript variable.

    0chainparams.cpp: In constructor ‘SigNetParams::SigNetParams(const ArgsManager&)’:
    1chainparams.cpp:284:52: error: ‘blockscript’ was not declared in this scope
    2         LogPrintf("Signet with block script %s\n", blockscript[0]);
    3                                                    ^~~~~~~~~~~
    4make[2]: *** [Makefile:10152: libbitcoin_common_a-chainparams.o] Error 1
    

    blockscript is only visible within the else statement above.


    kallewoof commented at 8:51 am on May 6, 2020:
    Thanks, yeah I failed at a rebase. Fixed now.
  106. kallewoof force-pushed on May 6, 2020
  107. kallewoof force-pushed on May 6, 2020
  108. jonatack commented at 1:55 pm on May 6, 2020: member

    Tested re-ACK d4e204beb795319

    If you retouch or as a follow-up: more explanation about -signet_hrp and #18267 (review), and maybe consider the change in #18267 (review) e.g. -signet_seednode=<ip> etc.

  109. in src/chainparamsbase.cpp:28 in d4e204beb7 outdated
    23@@ -23,6 +24,10 @@ void SetupChainParamsBaseOptions()
    24     gArgs.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    25     gArgs.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    26     gArgs.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    27+    gArgs.AddArg("-signet", "Use the signet chain. Note that the network is defined by the signet_blockscript parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    28+    gArgs.AddArg("-signet_blockscript", "Blocks must satisfy the given script to be considered valid (only for signet networks)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
    


    MarcoFalke commented at 3:14 pm on May 6, 2020:
    It seems that this also changes the network magic, so this is a switch to run a different signet?

    kallewoof commented at 6:17 am on May 7, 2020:

    Yep, the blockscript defines the network magic.

    https://github.com/bitcoin/bips/blob/master/bip-0325.mediawiki#message-start

    Edit: to clarify, each blockscript is considered its own signet, which is why the network magic depends on the blockscript.


    MarcoFalke commented at 1:26 pm on May 7, 2020:

    Wasn’t the point of signet to be a single network and everyone could choose their keys to decide which reorg to “follow”?

    E.g. there are different keys like:

    • no_regorgs_key: signs every block, advancing height by 1 every 10 min, never decreasing height. This is the “true” signet chain
    • 10_reorg_key: reorgs 10 blocks every 20 blocks, then reorgs back to to the “true” chain
    • 101_reorg_key: reorgs 100 blocks every 200 blocks, then back to the “true” chain

    kallewoof commented at 6:24 pm on May 7, 2020:

    That’s on the map, for sure, but it adds complexity I’m trying to avoid in the initial release.

    As for the single network, yes! The thing is, people are unpredictable, and I can imagine we will end up switching networks on rare occasions; in the end, we may have some form of org handling the privkeys to sign blocks, but initially at least, it will probably be me volunteering to do it, and if I go missing or evil or something, it should be trivial to switch to a new setup (later on, I’d like to make this also possible to switch without having to reset the chain, but that’s not MVP stuff). There may also be Bitcoin related companies (e.g. exchanges) who want to have their own signets in addition to the existing one.

    The approach here is discussed in #16411 (comment) but essentially, we are using the same genesis block for all signets, and instead switching the network magic to avoid mixing signets up.

  110. kallewoof force-pushed on May 7, 2020
  111. kallewoof commented at 6:20 am on May 7, 2020: member

    ~Fixed documentation for -signet_hrp (said suffix, meant prefix).~

    Removed -signet_hrp (see #18267 (review))

  112. kallewoof force-pushed on May 7, 2020
  113. brakmic commented at 11:44 am on May 7, 2020: contributor

    re-ACK 647c10fe82adcd5c4721424718afc67ce51b6c83

    Btw., I created a simple bash script for automatic building & running of Signet binaries on my 64bit Raspberry Pi. Should be able to run on other Linux systems as well.

    https://gist.github.com/brakmic/37c7618ad6e8bb33c6a271fe682e38a3

    Could maybe help to get more people to install and run Bitcoin Signet. :)

    The script also asks for sBTC from your faucet via cURL, but currently there is an 502 HTTP error, so I couldn’t test it thoroughly.

  114. kallewoof commented at 6:26 pm on May 7, 2020: member
    @brakmic Wow, very nice! Also, thanks for the heads up on 502 error. Investigating that.
  115. kallewoof commented at 6:41 pm on May 7, 2020: member

    @brakmic Tried that script on a MacBook, but it doesn’t like some of the arguments (see below). Still, very cool :)

    (And the faucet is back up.)

     0$ bash ./runsignet.sh
     1tr: Illegal byte sequence
     2tr: Illegal byte sequence
     3Creating backup datadir /Users/me/Downloads/signet/data/signet_datadir_backup
     4mkdir: /Users/me/Downloads/signet/data: No such file or directory
     5mkdir: /Users/me/Downloads/signet/data/signet_datadir_backup: No such file or directory
     6mkdir: /Users/me/Downloads/signet/data/signet_datadir_backup/signet: No such file or directory
     7Creating default bitcoin.conf
     8./runsignet.sh: line 44: /Users/me/Downloads/signet/data/signet_datadir_backup/bitcoin.conf: No such file or directory
     9-----------------------------------
    10  Getting Bitcoin Signet sources
    11-----------------------------------
    12Cloning into '/Users/me/Downloads/signet/data/signet'...
    13remote: Enumerating objects: 9, done.
    14remote: Counting objects: 100% (9/9), done.
    15remote: Compressing objects: 100% (7/7), done.
    16^Cceiving objects:  11% (19511/167291), 21.00 MiB | 4.31 MiB/s
    17$ ls
    18data          runsignet.sh  runsignet.sh~
    19$ ls data
    
  116. brakmic commented at 6:43 pm on May 7, 2020: contributor

    @brakmic Tried that script on a MacBook, but it doesn’t like some of the arguments (see below). Still, very cool :)

    Oh, I didn’t test it on a Mac. But as I luckily have one, I’ll investigate it and come back to you (with a hopefully updated version) :)

    (And the faucet is back up.)

    Awesome! Thanks! Now I can test the last few commands from the script.

  117. brakmic commented at 10:42 pm on May 7, 2020: contributor

    @brakmic Tried that script on a MacBook, but it doesn’t like some of the arguments (see below). Still, very cool :)

    0$ bash ./runsignet.sh
    1tr: Illegal byte sequence
    2tr: Illegal byte sequence
    

    Have fixed it. The problem was in random password/username generation sequence.

    tr on macOS doesn’t like certain byte sequences because they aren’t valid UTF-8.

    The script now simply takes the current dir as its root. No extra changes needed.

    Gist: https://gist.github.com/brakmic/37c7618ad6e8bb33c6a271fe682e38a3

    However, I wasn’t able to beg for coins from your server. Will check it later. But that’s not that important. ;)

  118. kallewoof commented at 6:20 am on May 8, 2020: member

    @brakmic Works better now! The rpcpassword ended up with a ‘#’ in it, though, so this happened:

    0Error: Error reading configuration file: parse error on line 8, using # in rpcpassword can be ambiguous and should be avoided
    
  119. brakmic commented at 6:33 am on May 8, 2020: contributor

    @brakmic Works better now! The rpcpassword ended up with a ‘#’ in it, though, so this happened:

    0Error: Error reading configuration file: parse error on line 8, using # in rpcpassword can be ambiguous and should be avoided
    

    Ah, yes, I was too generous with available random chars for user/pwd. Fixed now. Thanks!

  120. kallewoof commented at 6:49 am on May 8, 2020: member

    Works better this time; though it needs to wait for the node to finish starting up before it continues (am seeing a bunch of

    0error code: -28
    1error message:
    2Loading wallet...
    

    now).

  121. brakmic commented at 7:37 am on May 8, 2020: contributor

    Works better this time; though it needs to wait for the node to finish starting up before it continues (am seeing a bunch of

    0error code: -28
    1error message:
    2Loading wallet...
    

    now).

    I think it’s because the script was too fast for the IBD. Have put a simple “sleep 10s”, just to let it download a few blocks.

    Also, on macOS there is no manual BDB4 compilation anymore, only on Raspberry (no official packages in Raspbian repo)

    But somehow I can’t get the returned address to query for coins, which btw. now works. I can send a post to your server. :)

  122. kallewoof commented at 7:50 am on May 8, 2020: member
    @brakmic Check out https://gist.github.com/carnhofdaki/60edef577f637ef2dbf4d244e4e279c2 by @carnhofdaki – it does some fancy stuff to hold until the server is up. (I think that script is outdated now, but not sure. See #16411 (comment) for context.)
  123. brakmic commented at 7:53 am on May 8, 2020: contributor

    @brakmic Check out https://gist.github.com/carnhofdaki/60edef577f637ef2dbf4d244e4e279c2 by @carnhofdaki – it does some fancy stuff to hold until the server is up. (I think that script is outdated now, but not sure. See #16411 (comment) for context.)

    Thanks. I was wrong: the address got generated. The message “missing address” comes from your server, so I have somehow messed up the form data. :)

  124. brakmic commented at 8:21 am on May 8, 2020: contributor

    @kallewoof

    Begging for coins from your faucet works now :)

    coin_faucet

  125. fjahr commented at 3:24 pm on May 8, 2020: member

    re-ACK 647c10fe82adcd5c4721424718afc67ce51b6c83

    Did full review of earlier commits and checked later ones for no significant changes. Most notable changes:

    • rebased
    • reverted move of GetWitnessCommitmentIndex
    • moved GetWitnessCommitmentSection to signet.cpp
    • removed RPC -signet_hrp
    • introduced g_signet_blockscript
    • more nits addressed

    Tests passed locally.

  126. jonatack commented at 5:17 pm on May 10, 2020: member

    I think it’s because the script was too fast for the IBD. Have put a simple “sleep 10s”, just to let it download a few blocks.

    Even simpler, add -rpcwait to the CLI command?

  127. jonatack commented at 5:28 pm on May 10, 2020: member

    Code review re-ACK 647c10f only change since previous review is removal of the -signet_hrp “human-readable part” user option, per git diff d4e204b 647c10f

     0$ git diff d4e204b 647c10f
     1diff --git a/src/chainparams.cpp b/src/chainparams.cpp
     2index 797b22fbe9..669010c53e 100644
     3--- a/src/chainparams.cpp
     4+++ b/src/chainparams.cpp
     5@@ -325,7 +325,7 @@ public:
     6         base58Prefixes[EXT_PUBLIC_KEY] = {0x04, 0x35, 0x87, 0xCF};
     7         base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94};
     8 
     9-        bech32_hrp = "sb" + args.GetArg("-signet_hrp", "");
    10+        bech32_hrp = "sb";
    11 
    12         fDefaultConsistencyChecks = false;
    13         fRequireStandard = true;
    14diff --git a/src/chainparamsbase.cpp b/src/chainparamsbase.cpp
    15index 4d5485e38a..78a87371bb 100644
    16--- a/src/chainparamsbase.cpp
    17+++ b/src/chainparamsbase.cpp
    18@@ -26,7 +26,6 @@ void SetupChainParamsBaseOptions()
    19     gArgs.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    20     gArgs.AddArg("-signet", "Use the signet chain. Note that the network is defined by the signet_blockscript parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    21     gArgs.AddArg("-signet_blockscript", "Blocks must satisfy the given script to be considered valid (only for signet networks)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
    22-    gArgs.AddArg("-signet_hrp", "Human readable part of bech32 address (suffixed by \"sb\"; default = \"\")", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    23     gArgs.AddArg("-signet_seednode", "Specify a seed node for the signet network (may be used multiple times to specify multiple seed nodes)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
    24 }
    
  128. kallewoof commented at 6:18 am on May 11, 2020: member
    There are some really nice patches on top of this PR at https://github.com/ajtowns/bitcoin/commits/202005-signet-tweaks by @ajtowns – hopefully those can be in a follow-up PR, as I think this PR is close to merge-ready (if not, I guess I can merge them in here; we’ll see).
  129. MarcoFalke commented at 11:53 am on May 11, 2020: member

    @kallewoof I think for consensus changes the bar is a bit higher than for wallet changes (or other changes) and we should aim to get them right the first time with as little back-and-forth on the way there.

    The non-wip commits seem to get rid of quite a few chunks of code (and globals), so I wouldn’t mind having them squashed (with the appropriate Co-Author tag) into the commits here before merge.

  130. kallewoof force-pushed on May 12, 2020
  131. kallewoof commented at 10:40 am on May 12, 2020: member
    Merged all except WIP commit into current branch.
  132. fjahr commented at 4:09 pm on May 23, 2020: member

    re-ACK ae117d836a3371d294b03f73dff33604d1112702

    Only changes since my last review were replacing the blockscript global with the signet challenge in the consensus params and getting rid of the CheckBlockSolution function.

  133. in src/signet.h:14 in 7168395ea6 outdated
     9+#include <config/bitcoin-config.h>
    10+#endif
    11+
    12+#include <consensus/params.h>
    13+
    14+#include <stdint.h>
    


    MarcoFalke commented at 9:40 pm on May 23, 2020:

    In commit 7168395ea63d33075a60369f5778eb635208beba

    Are all these includes really needed? I’ve removed all non-std-lib includes and everything compiles fine with this diff:

     0diff --git a/src/signet.h b/src/signet.h
     1index b9946f950a..679df85c5d 100644
     2--- a/src/signet.h
     3+++ b/src/signet.h
     4@@ -5,18 +5,16 @@
     5 #ifndef BITCOIN_SIGNET_H
     6 #define BITCOIN_SIGNET_H
     7 
     8-#if defined(HAVE_CONFIG_H)
     9-#include <config/bitcoin-config.h>
    10-#endif
    11-
    12-#include <consensus/params.h>
    13-
    14-#include <stdint.h>
    15+#include <cstdint>
    16+#include <vector>
    17 
    18 class CBlock;
    19 class CScript;
    20 class uint256;
    21 struct CMutableTransaction;
    22+namespace Consensus {
    23+struct Params;
    24+}
    25 
    26 constexpr uint8_t SIGNET_HEADER[4] = {0xec, 0xc7, 0xda, 0xa2};
    27 
    

    kallewoof commented at 8:21 am on May 25, 2020:
    Not sure about the namespace/struct part, but otherwise sounds reasonable. Updated to this.
  134. in src/signet.cpp:45 in 7168395ea6 outdated
    40+    std::vector<uint256> leaves;
    41+    leaves.resize(block.vtx.size());
    42+    {
    43+        // find and delete signet signature
    44+        CMutableTransaction mtx(*block.vtx.at(0));
    45+        SetWitnessCommitmentSection(mtx, SIGNET_HEADER, std::vector<uint8_t>{});
    


    MarcoFalke commented at 9:41 pm on May 23, 2020:

    in commit 7168395ea63d33075a60369f5778eb635208beba

    I think the ignored return value should be documented

    0        (void)SetWitnessCommitmentSection(mtx, SIGNET_HEADER, std::vector<uint8_t>{});
    
  135. in src/signet.cpp:19 in 7168395ea6 outdated
    14+#include <util/system.h>
    15+
    16+int GetWitnessCommitmentIndex(const CBlock& block);
    17+template<typename T> int GetWitnessCommitmentIndex(const T& tx);
    18+
    19+static constexpr int NO_WITNESS_COMMITMENT{-1}; // note: this is copied from validation.h, to avoid a circular dependency issue
    


    MarcoFalke commented at 9:48 pm on May 23, 2020:

    In commit 7168395ea63d33075a60369f5778eb635208beba

    I don’t think we solve circular dependencies by copy pasting parts of the dependencies into each other. Doing so slightly defeats the purpose of having separate modules. There are at least two options:

    • Ignore the circular dependency for now (and document it as an exception in the linter)
    • Move the witness commitment stuff to a new “module”. I think libconsensus consensus/witness_commitment (or similar) could make sense. This would also help the miner not depend on the server (in a theoretical world where the miner is a separate util binary, where all state is passed in), but only on libconsensus.

    kallewoof commented at 8:21 am on May 25, 2020:

    Moving to avoid duplicate makes sense; honestly not sure what I was thinking here.

    I’m not sure it warrants a new module, so I’ve put it in consensus/validation.h for now. If you really do think it should be in a separate file, I’ll move it.

  136. kallewoof force-pushed on May 25, 2020
  137. in src/signet.cpp:18 in 988b8c065b outdated
    13+#include <script/interpreter.h>
    14+#include <script/standard.h>        // MANDATORY_SCRIPT_VERIFY_FLAGS
    15+#include <util/system.h>
    16+
    17+int GetWitnessCommitmentIndex(const CBlock& block);
    18+template<typename T> int GetWitnessCommitmentIndex(const T& tx);
    


    MarcoFalke commented at 11:13 am on May 26, 2020:
    Same for those two as well, obviously ;)

    kallewoof commented at 4:45 am on May 27, 2020:
    Oh… yeah, okay. I moved the declarations as well (from src/validation.h).
  138. kallewoof force-pushed on May 27, 2020
  139. in src/script/interpreter.cpp:1504 in 72758ea816 outdated
    1500@@ -1501,6 +1501,18 @@ bool GenericTransactionSignatureChecker<T>::CheckSequence(const CScriptNum& nSeq
    1501 template class GenericTransactionSignatureChecker<CTransaction>;
    1502 template class GenericTransactionSignatureChecker<CMutableTransaction>;
    1503 
    1504+bool SimpleSignatureChecker::CheckSig(const std::vector<unsigned char>& vchSigIn, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const
    


    instagibbs commented at 1:19 pm on May 29, 2020:
    scriptCode unused ever, maybe drop a comment saying it’s just there due to inheritance?

    kallewoof commented at 5:22 am on May 31, 2020:
    Do we usually do that? I know e.g. in other languages unused parameters are marked by giving them an underscore prefix, but I don’t think I’ve seen this in Bitcoin Core.

    MarcoFalke commented at 10:49 am on May 31, 2020:
    In some places we omit unused args (like scriptCode), in others we mark it unused like /* scriptCode */. In both cases only the type is kept, which indicates that the value can not be used and is unused. Not sure if there is a guideline.

    kallewoof commented at 4:49 am on June 1, 2020:
    I updated the code to exclude the name (scriptCode) in declaration/definition.
  140. in src/signet.cpp:28 in 89ef14d17e outdated
    23+    }
    24+    SimpleSignatureChecker bsc(GetSignetHash(block));
    25+    CScript challenge(consensusParams.signet_challenge.begin(), consensusParams.signet_challenge.end());
    26+    CScript solution = CScript(signet_data.begin(), signet_data.end());
    27+
    28+    if (!VerifyScript(solution, challenge, nullptr, MANDATORY_SCRIPT_VERIFY_FLAGS, bsc)) {
    


    instagibbs commented at 1:39 pm on May 29, 2020:

    Have you given thought to the other script verification flags? People can malleate the witness to make it larger.

    Alternative is to cap the signature size by consensus param.


    kallewoof commented at 5:36 am on May 31, 2020:
    People can already kind of malleate blocks by changing the nonce, since it isn’t committed to in the signet hash, so it feels like going for the same level of restrictions as in the block checks is sensible, I think. Thoughts @ajtowns ?

    ajtowns commented at 3:08 am on July 20, 2020:
    Since you can already malleate the nonce to get a different block, and can’t build on the block without being able to do a valid signature, I guess malleation isn’t really a concern, but I think extra flags would make sense. I wonder if exporting validation.cpp:GetBlockScriptFlags and calling it with the genesis block (or making it accept pindexPrev and calling it with nullptr) to get the script flags would make sense.
  141. in src/validation.cpp:3330 in 4991f1545b outdated
    3324@@ -3318,6 +3325,11 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
    3325     if (!CheckBlockHeader(block, state, consensusParams, fCheckPOW))
    3326         return false;
    3327 
    3328+    // Signet only: check block solution, unless this is the genesis block
    3329+    if (consensusParams.signet_blocks && fCheckPOW && block.GetHash() != consensusParams.hashGenesisBlock && !CheckBlockSolution(block, consensusParams)) {
    3330+        return false;
    


    instagibbs commented at 1:44 pm on May 29, 2020:
    could return an explicit state.Invalid line for better debugging?

    kallewoof commented at 5:39 am on May 31, 2020:
    Sounds good!
  142. in src/signet.cpp:20 in 89ef14d17e outdated
    15+#include <util/system.h>
    16+
    17+// Signet block solution checker
    18+bool CheckBlockSolution(const CBlock& block, const Consensus::Params& consensusParams)
    19+{
    20+    std::vector<uint8_t> signet_data;
    


    instagibbs commented at 1:53 pm on May 29, 2020:

    Maybe too late but: Turning this into a witness stack + adding some validation flags would allow new scripts to be deployed on new networks without any additional work. We do this in Elements which means you can deploy segwit-style scripts, taproot etc.

    Ok there’s one catch in that the witnessScript has to be known by the blocksigner to fill out the witness stack, but relatively small burden.


    kallewoof commented at 5:40 am on May 31, 2020:
    I think @ajtowns is working on a patch to Signet to do precisely this, but I think I’m leaning towards keeping it simple for now. I mean, taproot in signet blocks doesn’t really make any sense, or open up any possibilities, AFAICT.
  143. instagibbs approved
  144. instagibbs commented at 2:02 pm on May 29, 2020: member

    code review ACK https://github.com/bitcoin/bitcoin/pull/18267/commits/d50b888aeb863532e37b3fb138c66bba0025d91a

    Pleasingly simple and small PR. I think it’s worth considering changing up the block witness format a bit to be forward-supporting of script updates, but is also simply a “nice to have” since this setup is only for test networks and doesn’t involve a dynamic signer set. You could just release a new set of functionality for new type of signet for little cost later and still support “legacy” signet.

  145. kallewoof force-pushed on May 31, 2020
  146. fanquake deleted a comment on May 31, 2020
  147. fanquake deleted a comment on May 31, 2020
  148. kallewoof force-pushed on Jun 1, 2020
  149. kallewoof force-pushed on Jun 1, 2020
  150. fjahr commented at 7:59 pm on June 1, 2020: member

    re-ACK 03ac167829d555de456d684f6580c6429253c7f0

    Changes since my last review only addressed various review comments that came up in between.

  151. in src/test/key_io_tests.cpp:142 in 03ac167829 outdated
    135@@ -136,7 +136,7 @@ BOOST_AUTO_TEST_CASE(key_io_invalid)
    136         std::string exp_base58string = test[0].get_str();
    137 
    138         // must be invalid as public and as private key
    139-        for (const auto& chain : { CBaseChainParams::MAIN, CBaseChainParams::TESTNET, CBaseChainParams::REGTEST }) {
    140+        for (const auto& chain : { CBaseChainParams::MAIN, CBaseChainParams::TESTNET, CBaseChainParams::SIGNET, CBaseChainParams::REGTEST }) {
    141             SelectParams(chain);
    142             destination = DecodeDestination(exp_base58string);
    143             BOOST_CHECK_MESSAGE(!IsValidDestination(destination), "IsValid pubkey in mainnet:" + strTest);
    


    pinheadmz commented at 12:58 pm on July 2, 2020:
    This may be out of scope for this PR, but as long as we’re here - should the word “mainnet” actually be replaced by the name of the network selected in the for loop? chain.NetworkIDString()

    kallewoof commented at 3:44 am on July 13, 2020:
    That’s a good point, yeah. I think it deserves a dedicated PR though, even if a one liner.
  152. in src/chainparams.cpp:280 in 03ac167829 outdated
    274+            if (blockscript.size() != 1) {
    275+                throw std::runtime_error(strprintf("%s: -signet_blockscript cannot be multiple values.", __func__));
    276+            }
    277+            bin = ParseHex(blockscript[0]);
    278+
    279+            LogPrintf("Signet with block script %s\n", blockscript[0]);
    


    pinheadmz commented at 1:03 pm on July 2, 2020:
    Do you think it would be helpful to print the decoded ASM of the script as well?

    kallewoof commented at 3:46 am on July 13, 2020:
    From personal experience, this is a check point to ensure you’re not accidentally running on the wrong signet, in which case the ASM won’t really help any.
  153. in src/chainparams.cpp:269 in 03ac167829 outdated
    263+        std::vector<uint8_t> bin;
    264+        vSeeds.clear();
    265+
    266+        if (!args.IsArgSet("-signet_blockscript")) {
    267+            LogPrintf("Using default signet network\n");
    268+            bin = ParseHex("512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be43051ae");
    


    pinheadmz commented at 1:04 pm on July 2, 2020:

    OP_1 03ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430 OP_1 OP_CHECKMULTISIG

    Why did you choose 1-of-1 multisig instead of a single OP_CHECKSIG ?


    kallewoof commented at 3:47 am on July 13, 2020:
    Mostly as I expect there to be backup signers and such, so having it on a multisig setup from the start seems sensible.
  154. in src/signet.cpp:59 in 03ac167829 outdated
    54+}
    55+
    56+bool GetWitnessCommitmentSection(const CBlock& block, const uint8_t header[4], std::vector<uint8_t>& result)
    57+{
    58+    int cidx = GetWitnessCommitmentIndex(block);
    59+    if (cidx == NO_WITNESS_COMMITMENT) return false;
    


    pinheadmz commented at 1:19 pm on July 2, 2020:

    This may be more of a note on BIP325 in general, but on mainnet currently it is not invalid to produce a block with no transactions and therefore no witness commitment in the coinbase tx.

    I found a recent example: https://blockstream.info/block/0000000000000000000e77c9c81498be9a4aa226d4be35a19a0fb11d99331d6a

    Why does the spec depend on a witness commitment instead of new OP_RETURN for this purpose?


    kallewoof commented at 3:51 am on July 13, 2020:

    That’s a good point, but then again, it IS allowed to put a witness commitment in a block with no transactions (as evidenced by most of the signet blocks right now), so it doesn’t feel like a huge deal in the end. After all, the signet blocks will “suffer” from the extra overhead with empty blocks, but that seems like an acceptable trade-off.

    The question is if it’s simpler to do an OP_RETURN instead of a witness commitment extension. @sipa or someone, would love your input on this.

  155. in src/signet.h:41 in 03ac167829 outdated
    36+/**
    37+ * Attempt to get the data for the section with the given header in the witness commitment of the block.
    38+ *
    39+ * Returns false if header was not found. The data (excluding the 4 byte header) is written into result if found.
    40+ */
    41+bool GetWitnessCommitmentSection(const CBlock& block, const uint8_t header[4], std::vector<uint8_t>& result);
    


    practicalswift commented at 2:39 pm on July 2, 2020:
    Array-to-pointer safety nit: What about const std::array<uint8_t, 4> (instead of const uint8_t header[4]) and giving SIGNET_HEADER the same treatment?

    ajtowns commented at 11:08 pm on July 8, 2020:
    I think using a span here would work even better.

    kallewoof commented at 4:09 am on July 13, 2020:

    Trying to switch the SIGNET_HEADER declaration into a Span, I end up with this ugly beast. Anything I can do to improve that? Going to go with @practicalswift’s suggestion for now. :)

    0const Span<const uint8_t> SIGNET_HEADER =
    1  MakeSpan(
    2    (const std::vector<const uint8_t>)std::vector<const uint8_t>{0xec, 0xc7, 0xda, 0xa2}
    3  );
    

    sipa commented at 4:16 am on July 13, 2020:

    No, Span does not store any data. Doing that would be illegal (it’s creating a vector, storing a pointer to its data in a Span, and then throwing the vector away).

    Just do this:

    0const uint8_t SIGNET_HEADER[4] = {0xec, 0xc7, 0xda, 0xa2};
    

    (or std::array, or std::vector, or whatever container you prefer…)

    And make GetWitnessCommitmentSection accept a Span<const uint8_t>. You can just pass SIGNET_HEADER as argument to GetWitnessCommitmentSection - it’ll be converted to a Span on the fly.


    kallewoof commented at 4:32 am on July 13, 2020:
    Gotcha! Thanks.
  156. pinheadmz commented at 2:48 pm on July 2, 2020: member

    Concept ACK

    Build OK (OSX), Tests pass locally OK functional + unit.

    This is all very cool, going to run a node in a second but I have some general questions:

    1. Should there be additional test cases for address and key strings for signet in src/test/data/key_io_valid.json ?
    2. I’m unclear what the outcome would be if the challenge script had CSV or CLTV – does the method from BaseSignatureChecker just return false? Is there any reason to expand those methods into the signet checker? Perhaps grab the nSequence or nLocktime values from the coinbase TX?
    3. Re: hard-coded public key and IP address for “the” signet. I understand the idea here is that groups of developers can launch their own signet chain with unique parameters. Still, having a default identity (pubkey, IP) seems potentially awkward. I wonder if the private key you set should be revealed? Or if the “default” challenge should be otherwise more accessible/permissionless? Having a single hard-coded seed might require on going maintenance.
  157. instagibbs commented at 2:56 pm on July 2, 2020: member

    I wonder if the private key you set should be revealed?

    This would turn into testnet with mining essentially. Private key being secret is important.

  158. pinheadmz commented at 3:02 pm on July 2, 2020: member

    Running on signet now. I noticed in the log all blocks during IBD were reported as progress=1.000000

    I wonder if it makes sense to add logic in GuessVerificationProgress() that just compares current wall-clock time to the timestamp in the chain tip (I think this is how we used to do it before hard coding an estimated TX/s into chainparams)

    Or maybe there’s a value equivalent to “1 tx every ten minutes” that can be used to fudge it:

    https://github.com/bitcoin/bitcoin/blob/7027c67cac852b27c6d71489e4135fabdd624226/src/chainparams.cpp#L159-L164

  159. instagibbs commented at 3:04 pm on July 2, 2020: member

    Might want to look how Elements(Liquid nodes) does it. Should be directly usable?

    On Thu, Jul 2, 2020, 11:02 AM Matthew Zipkin notifications@github.com wrote:

    Running on signet now. I noticed in the log all blocks during IBD were reported as progress=1.000000

    I wonder if it makes sense to add logic in GuessVerificationProgress() that just compares current wall-clock time to the timestamp in the chain tip (I think this is how we used to do it before hard coding an estimated TX/s into chainparams)

    Or maybe there’s a value equivalent to “1 tx every ten minutes” that can be used to fudge it:

    https://github.com/bitcoin/bitcoin/blob/7027c67cac852b27c6d71489e4135fabdd624226/src/chainparams.cpp#L159-L164

    — You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/18267#issuecomment-653059667, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFU5DFVAK6YJBTNO3TSDRZSORZANCNFSM4LCDORNQ .

  160. pinheadmz commented at 3:16 pm on July 2, 2020: member

    I wonder if the private key you set should be revealed?

    This would turn into testnet with mining essentially. Private key being secret is important.

    OK - so this PR represents one single signet that everyone will use instead of testnet? If so then I understand keeping the key private, except that it could be a burden on the single “host”.

    My impression is that groups of developers will launch their own signets with custom parameters for various reasons, and this PR just defines a generic framework. I guess it just looks funny to hard code one person’s key in consensus code, even for a test network. I’d expect to see that like more of an example in a test instead, with the actual production code left more generic.

  161. in src/chainparams.cpp:335 in 03ac167829 outdated
    326+        base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94};
    327+
    328+        bech32_hrp = "sb";
    329+
    330+        fDefaultConsistencyChecks = false;
    331+        fRequireStandard = true;
    


    pinheadmz commented at 3:45 pm on July 2, 2020:
    Why require standard for signet? This is false for testnet. Is the idea just to be more mainnet-like?

    kallewoof commented at 4:12 am on July 13, 2020:
    Yeah, one of the design goals with signet is to be as close to mainnet as possible. The further it deviates, the less useful it is for people who want to make sure their stuff works.

    ajtowns commented at 8:15 am on September 8, 2020:
    Note that the fRequireStandard is in validation.cpp and initialised in init.cpp based on -acceptnonstdtxn – so users can individually override this setting if they want. [ie, the value that actually controls whether non std txs get into the mempool is in validation.cpp, the setting here changes the member variable in chainparams which is just used as a default for initialising the global from validation]
  162. pinheadmz commented at 4:06 pm on July 2, 2020: member

    Connected to signet, got coins from faucet, did a sendmany to a dozen internal wallet addresses - all good! Had to set fallbackfee for this network to send.

    Also tried starting a signet with -signet_blockscript=51 (OP_TRUE) and still got block solution missing when attempting to generate. This branch doesn’t have the mining code yet but I thought I could trick it by using an “anyone can mine” script :-)

  163. in src/consensus/validation.h:154 in fe5b704bdd outdated
    149+
    150+/** Compute at which vout of the block's coinbase transaction the witness commitment occurs, or -1 if not found */
    151+int GetWitnessCommitmentIndex(const CBlock& block);
    152+
    153+/** Compute at which vout of the given coinbase transaction the witness commitment occurs, or -1 if not found */
    154+template<typename T> int GetWitnessCommitmentIndex(const T& tx);
    


    ajtowns commented at 7:35 pm on July 2, 2020:
    I think if you’re moving these functions into consensus/ then they should either be inline (which seems reasonable enough) or should be in a new consensus/validation.cpp as otherwise libconsensus won’t be self-contained.

    kallewoof commented at 4:22 am on July 13, 2020:
    Experimenting with making these inline and contained in consensus/validation.h.
  164. kallewoof commented at 4:17 am on July 13, 2020: member

    @pinheadmz

    OK - so this PR represents one single signet that everyone will use instead of testnet? If so then I understand keeping the key private, except that it could be a burden on the single “host”.

    My impression is that groups of developers will launch their own signets with custom parameters for various reasons, and this PR just defines a generic framework. I guess it just looks funny to hard code one person’s key in consensus code, even for a test network. I’d expect to see that like more of an example in a test instead, with the actual production code left more generic.

    That was my initial idea too, but current trend is that there is a super-signet that basically has most of the proposed features, and people choose to activate whatever they want to test out. Due to the nature of soft forks, inactive features will be silently ignored/accepted in blocks.

    Of course there may be spin-off signets, esp. as people want to test stuff out that hasn’t gotten far enough to be added to the above yet, and there may be “LTS signets” for people who really only want to have something that runs forever, like mainnet.

  165. kallewoof commented at 4:25 am on July 13, 2020: member
    @practicalswift @ajtowns @pinheadmz and others, thanks a lot for review, and sorry about delay in responding. I’ve pushed two to-squash commits. Feedback welcome!
  166. kallewoof force-pushed on Jul 13, 2020
  167. kallewoof force-pushed on Jul 13, 2020
  168. kallewoof force-pushed on Jul 14, 2020
  169. pinheadmz commented at 3:22 pm on July 14, 2020: member

    @instagibbs

    Might want to look how Elements(Liquid nodes) does it. Should be directly usable?

    Looks like since Liquid has “guaranteed” block time (signet will too, right?) IBD progress is computed from the timestamp of the chain tip and current time.

    https://github.com/Blockstream/liquid/commit/5652022f4b21c8dd6bb595ede4c35e8ec62d5765#diff-24efdb00bfbe56b140fb006b562cc70bR5321-R5326

  170. pinheadmz commented at 4:00 pm on July 14, 2020: member

    Confirmed recent rebase is minimal (just added 2 commits, not sure why other commit hashes changed or why github identified it as a force-push?)

    Still builds OK on OSX, unit tests pass. Synced to signet OK.

    Just waiting to hear more opinions on the commitment location before ACKing: https://github.com/bitcoin/bitcoin/pull/18267/commits/9fd83904dd745be7ffaf51262e344e6c3dcf07cc#r453420748

    Functional tests failed:

    0wallet_basic.py failed, Duration: 62 s
    1wallet_address_types.py failed, Duration: 65 s
    

    Both were AssertionError: [node 4] Unable to connect to bitcoind after 60s maybe just a CPU crunch on my own machine

    I can post the stack traces if you want

  171. kallewoof commented at 11:10 pm on July 14, 2020: member

    @pinheadmz

    Thanks for the re-review!

    Looks like since Liquid has “guaranteed” block time (signet will too, right?) IBD progress is computed from the timestamp of the chain tip and current time.

    Signet will be using proof of work as well, so it’s not guaranteed per se, but it should be very unusual to see big discrepancies in block times due to the limited difficulty (currently plan to mine 1 min and delay 9 mins, but there may be other types in the future).

    Confirmed recent rebase is minimal (just added 2 commits, not sure why other commit hashes changed or why github identified it as a force-push?)

    This is because I had to rebase on master to get some changes to Span that were added since the last rebase.

    Those errors you are seeing are most likely unrelated. If they persist when you run again, let me know and I’ll look. :)

  172. adamjonas commented at 0:51 am on July 15, 2020: member
    For what it’s worth, I ran the functional tests for wallet_basic.py and wallet_address_types.py four times against 9fd83904d and didn’t get any failures.
  173. in src/script/interpreter.h:170 in e6ec7783d7 outdated
    165@@ -166,6 +166,17 @@ class BaseSignatureChecker
    166     virtual ~BaseSignatureChecker() {}
    167 };
    168 
    169+/** A low level signature checker.  Note that the caller must verify that the hash passed in relates to a known message (unlike for the other signature checkers). */
    170+class SimpleSignatureChecker : public BaseSignatureChecker
    


    ajtowns commented at 3:17 am on July 20, 2020:
    I’ve done a proposal to change from a special signature checker to reusing transaction signature checking at https://github.com/bitcoin/bips/pull/947 . @sdaftuar I think this might resolve your concern from #16653 (comment)
  174. kallewoof force-pushed on Jul 30, 2020
  175. kallewoof commented at 3:14 am on July 30, 2020: member
    This pull request has been updated to reflect the changes in https://github.com/bitcoin/bips/pull/947 – see https://github.com/ajtowns/bitcoin/commits/202007-signet for details on what changed (note: some commits were out of scope for this PR, such as generation, and have not been cherry-picked/merged here).
  176. kallewoof force-pushed on Jul 30, 2020
  177. kallewoof force-pushed on Jul 30, 2020
  178. DrahtBot added the label Needs rebase on Jul 30, 2020
  179. kallewoof force-pushed on Jul 31, 2020
  180. DrahtBot removed the label Needs rebase on Jul 31, 2020
  181. in src/signet.h:34 in 359badf294 outdated
    29+ * 2. It skips the nonce.
    30+ */
    31+class SignetTxs {
    32+private:
    33+    template<class T1, class T2>
    34+    SignetTxs(const T1& to_spend_, const T2& to_sign_) : to_spend{to_spend_}, to_sign{to_sign_} { }
    


    promag commented at 11:23 pm on August 1, 2020:

    359badf294c8fa632b7df479a3e146cf28658f6b

    nit, args can have the same name as members.


    kallewoof commented at 3:29 am on August 3, 2020:
    I personally find that confusing. I think renaming the instance vars to m_* is worthwhile, though, so will do that.
  182. in src/consensus/params.h:88 in 359badf294 outdated
    79@@ -80,6 +80,13 @@ struct Params {
    80     int64_t DifficultyAdjustmentInterval() const { return nPowTargetTimespan / nPowTargetSpacing; }
    81     uint256 nMinimumChainWork;
    82     uint256 defaultAssumeValid;
    83+
    84+    /**
    85+     * If true, witness commitments contain a payload equal to a Bitcoin Script solution
    86+     * to the signet challenge.
    87+     */
    88+    bool signet_blocks{false};
    


    promag commented at 11:33 pm on August 1, 2020:

    359badf294c8fa632b7df479a3e146cf28658f6b

    nit, just noting signet_blocks is only used in the next commit.

  183. in src/signet.h:32 in 359badf294 outdated
    31+class SignetTxs {
    32+private:
    33+    template<class T1, class T2>
    34+    SignetTxs(const T1& to_spend_, const T2& to_sign_) : to_spend{to_spend_}, to_sign{to_sign_} { }
    35+
    36+    static SignetTxs Create(const CBlock& block, const CScript& challenge);
    


    promag commented at 11:37 pm on August 1, 2020:

    359badf294c8fa632b7df479a3e146cf28658f6b

    Why have this separated? Could be in the to the constructor?


    ajtowns commented at 0:21 am on August 2, 2020:
    The member vars are CTransactions rather than CMutableTransactions, and are effectively const, so have to be list initialized, so the actual logic has to be moved elsewhere. (At least as far as I can see)

    promag commented at 0:53 am on August 2, 2020:
    Thanks for the explanation 👍
  184. in src/signet.h:41 in 359badf294 outdated
    36+    static SignetTxs Create(const CBlock& block, const CScript& challenge);
    37+
    38+public:
    39+    SignetTxs(const CBlock& block, const CScript& challenge) : SignetTxs(Create(block, challenge)) { }
    40+
    41+    CTransaction to_spend;
    


    promag commented at 11:39 pm on August 1, 2020:

    359badf294c8fa632b7df479a3e146cf28658f6b

    These could be const?

  185. in src/signet.cpp:78 in 359badf294 outdated
    73+    tx_spending.vin.emplace_back(COutPoint(), CScript(), 0);
    74+    tx_spending.vout.emplace_back(0, CScript(OP_RETURN));
    75+
    76+    // can't fill any other fields before extracting signet
    77+    // responses from block coinbase tx
    78+    {
    


    promag commented at 11:52 pm on August 1, 2020:

    359badf294c8fa632b7df479a3e146cf28658f6b

    Can’t find a reason for this block. Is this a leftover?


    ajtowns commented at 0:31 am on August 2, 2020:
    Yeah, it was making it possible to free up some data at one point, but now there’s nothing going on afterwards anymore.
  186. kallewoof commented at 3:34 am on August 3, 2020: member
    Addressed nits in to-squashies.
  187. kallewoof force-pushed on Aug 11, 2020
  188. kallewoof force-pushed on Aug 11, 2020
  189. kallewoof force-pushed on Aug 11, 2020
  190. kallewoof commented at 2:25 am on August 11, 2020: member
    • Updated to use new 1-of-2 key for the new Signet network (new 2nd key maintained by @ajtowns).
    • Trimmed down consensus/validation.h stuff, removing the transaction based GetWitnessCommitmentIndex variant
    • Squashed to-squashies (including the new stuff)
  191. kallewoof commented at 3:32 am on August 12, 2020: member

    Test network was restarted (Signet Global Test Net V) along with below services:

  192. JeremyRubin commented at 5:07 am on August 12, 2020: contributor
    CR-Ack – looked over, everything looks fine. Doesn’t seem to interfere with any non-signet code paths.
  193. in src/validation.cpp:1167 in 4b96892b06 outdated
    1162@@ -1162,6 +1163,12 @@ bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::P
    1163     if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
    1164         return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
    1165 
    1166+    // Signet only: check block solution, unless this is the genesis block
    1167+    if (consensusParams.signet_blocks && block.GetHash() != consensusParams.hashGenesisBlock && !CheckBlockSolution(block, consensusParams)) {
    


    ajtowns commented at 4:08 am on August 13, 2020:
    Any reason not to move the block.GetHash() != consensusParams.hashGenesisBlock check into CheckBlockSolution?

    kallewoof commented at 5:44 am on August 13, 2020:
    There was, but not any more – fixed, thanks!
  194. ajtowns approved
  195. ajtowns commented at 4:58 am on August 13, 2020: member
    ACK 4b96892b06a5f45e9dc1117dda0552822bfb5ce4
  196. kallewoof force-pushed on Aug 13, 2020
  197. in src/chainparams.cpp:330 in 96e3d1e451 outdated
    322+
    323+        base58Prefixes[PUBKEY_ADDRESS] = std::vector<unsigned char>{125};
    324+        base58Prefixes[SCRIPT_ADDRESS] = std::vector<unsigned char>{87};
    325+        base58Prefixes[SECRET_KEY] =     std::vector<unsigned char>{217};
    326+        base58Prefixes[EXT_PUBLIC_KEY] = {0x04, 0x35, 0x87, 0xCF};
    327+        base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94};
    


    maaku commented at 5:57 am on August 13, 2020:
    Why are these prefix values different from testnet/regtest?

    kallewoof commented at 6:12 am on August 13, 2020:
    Having them be the same might cause confusion when someone tries to use test/signet to transact on sig/testnet.. no biggie for regtest, which I assume is why (reg)test(net) share prefixes.

    kallewoof commented at 7:03 am on August 28, 2020:
    Switched to using testnet prefixes (including bech32 one).
  198. NicolasDorier commented at 6:09 am on August 13, 2020: contributor
    That’s better than before. ACK 96e3d1e . I wish we can get signet soon, it will help to test taproot for example.
  199. NicolasDorier commented at 8:40 am on August 13, 2020: contributor

    @kallewoof there is one thing I don’t like, but feel free to ignore my remark:

    If there is for example a new signet for taproot. People who wants to use it, need to get 3 command line parameters right.

    It would be great if it was all in one parameter say

    -signet=ab63bc...&signet.node.com such that people creating signet can just share a single string with everthing needed instead of 3 different parameters.

    Maybe not worth another round of review though.

  200. kallewoof commented at 9:05 am on August 13, 2020: member
    @NicolasDorier I think that makes perfect sense, but it can easily be a follow-up addition. (One that I will personally write, if nobody beats me to it!)
  201. jonatack commented at 9:19 am on August 13, 2020: member

    Built and manually retested following my steps in #18267 (comment) – the latest version still works as advertised.

    Now reviewing the code changes since my 4th review on May 10 at #18267 (comment).

    Edit: ugh, looks like a total re-review is needed.

  202. kallewoof commented at 9:25 am on August 13, 2020: member
    @jonatack Sorry about that. >< The changes are improvements, FWIW!
  203. in src/consensus/params.h:86 in 3d1b58f1ef outdated
    79@@ -80,6 +80,13 @@ struct Params {
    80     int64_t DifficultyAdjustmentInterval() const { return nPowTargetTimespan / nPowTargetSpacing; }
    81     uint256 nMinimumChainWork;
    82     uint256 defaultAssumeValid;
    83+
    84+    /**
    85+     * If true, witness commitments contain a payload equal to a Bitcoin Script solution
    86+     * to the signet challenge.
    


    fjahr commented at 3:12 pm on August 15, 2020:
    nit: Could add reference to BIP325 here
  204. in src/signet.cpp:132 in 3d1b58f1ef outdated
    127+        return true;
    128+    }
    129+
    130+    int cidx = GetWitnessCommitmentIndex(block);
    131+    if (cidx == NO_WITNESS_COMMITMENT) {
    132+        return error("CheckBlockSolution: Errors in block (no witness comittment)");
    


    fjahr commented at 4:14 pm on August 15, 2020:
    0        return error("CheckBlockSolution: Errors in block (no witness commitment)");
    
  205. in src/consensus/validation.h:178 in 3c22f5fcd8 outdated
    171+                vout.scriptPubKey[5] == 0xed) {
    172+                commitpos = o;
    173+            }
    174+        }
    175+    }
    176+    return commitpos;
    


    fjahr commented at 4:23 pm on August 15, 2020:

    nit: to consider if you feel like improving this a little more since it’s not strictly move-only anyway (github doesn’t highlight changes here, which is annoying. This drops the commitpos variable and changes for loop increment to conform with the style guide.)

     0    if (!block.vtx.empty()) {
     1        for (size_t o = 0; o < block.vtx[0]->vout.size(); ++o) {
     2            const CTxOut& vout = block.vtx[0]->vout[o];
     3            if (vout.scriptPubKey.size() >= MINIMUM_WITNESS_COMMITMENT &&
     4                vout.scriptPubKey[0] == OP_RETURN &&
     5                vout.scriptPubKey[1] == 0x24 &&
     6                vout.scriptPubKey[2] == 0xaa &&
     7                vout.scriptPubKey[3] == 0x21 &&
     8                vout.scriptPubKey[4] == 0xa9 &&
     9                vout.scriptPubKey[5] == 0xed) {
    10                return o;
    11            }
    12        }
    13    }
    14    return NO_WITNESS_COMMITMENT;
    

    kallewoof commented at 0:45 am on August 17, 2020:
    I wanted to do even more (e.g. loop from end and break on first encounter, rather than always going through all of them), but I refrained as I wanted minimal changes. [Edit: I think I’m misremembering something, because this code doesn’t do what I said (anymore?)] [Edit 2: your changes actually change the behavior. See the Segwit BIP on witness commitment.] Chicken-egg issue though, as we tend to discourage pure refactors, and we also tend to shy away from changing things unnecessarily in unrelated PRs…
  206. in src/signet.cpp:123 in 3d1b58f1ef outdated
    126+        // genesis block solution is always valid
    127+        return true;
    128+    }
    129+
    130+    int cidx = GetWitnessCommitmentIndex(block);
    131+    if (cidx == NO_WITNESS_COMMITMENT) {
    


    fjahr commented at 7:32 pm on August 15, 2020:

    nit: can be shortened (or make cidx const).

    0    if (GetWitnessCommitmentIndex(block) == NO_WITNESS_COMMITMENT) {
    
  207. in src/signet.cpp:82 in 3d1b58f1ef outdated
    77+    // responses from block coinbase tx
    78+
    79+    // find and delete signet signature
    80+    CMutableTransaction cb(*block.vtx.at(0));
    81+
    82+    int cidx = GetWitnessCommitmentIndex(block);
    


    fjahr commented at 7:34 pm on August 15, 2020:
    0    const int cidx = GetWitnessCommitmentIndex(block);
    
  208. fjahr approved
  209. fjahr commented at 11:33 pm on August 15, 2020: member

    re-ACK 96e3d1e45157cb8f2c7ecae8366fe076cc913742

    Tested by syncing with global signet and did full re-review of the code after studying changes in the BIP. I left a few nits, all of which I think can be ignored or left for follow-ups.

    Additionally, I found the use of the bad variable in Create and passing an error from Create to CheckBlockSolution via clearing the transactions a bit hard to parse. This may be too late now since several have ACKed the current approach but I would have suggested using an externally passed success variable. I have drafted this out here https://github.com/fjahr/bitcoin/commit/46e049f65b31a229a84f18a7d6120c9f08725c35 and it uses fewer lines of code and is a bit more readable IMHO. I think this should not be a behavior change and I was able to sync with this code. However, absolutely feel free to ignore.

  210. kallewoof commented at 1:02 am on August 17, 2020: member
    @fjahr Thanks for review! Adapted your bad-replacement with some modifications. I addressed most other things, but kept the cidx stuff as is for now.
  211. kallewoof force-pushed on Aug 17, 2020
  212. in src/signet.h:15 in 8b83160a74 outdated
    10+#include <primitives/block.h>
    11+#include <uint256.h>
    12+
    13+#include <cstdint>
    14+#include <vector>
    15+#include <array>
    


    jonatack commented at 6:59 am on August 17, 2020:

    9d19caf nit: sort ;) or just run clang-format on signet.h/signet.cpp

    0 #include <consensus/params.h>
    1-#include <primitives/transaction.h>
    2 #include <primitives/block.h>
    3+#include <primitives/transaction.h>
    4 #include <uint256.h>
    5 
    6+#include <array>
    7 #include <cstdint>
    8 #include <vector>
    9-#include <array>
    
  213. kallewoof force-pushed on Aug 17, 2020
  214. in src/util/system.cpp:931 in 33b2e22d5a outdated
    928     const bool is_chain_arg_set = IsArgSet("-chain");
    929 
    930-    if ((int)is_chain_arg_set + (int)fRegTest + (int)fTestNet > 1) {
    931-        throw std::runtime_error("Invalid combination of -regtest, -testnet and -chain. Can use at most one.");
    932+    if ((int)is_chain_arg_set + (int)fRegTest + (int)fTestNet + (int)fSigNet > 1) {
    933+        throw std::runtime_error("Invalid combination of -regtest, -testnet, -signet and -chain. Can use at most one.");
    


    jonatack commented at 8:27 am on August 17, 2020:

    6920af6 nit, consistent order

    0        throw std::runtime_error("Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.");
    
  215. in src/chainparamsbase.cpp:51 in 33b2e22d5a outdated
    47+        return MakeUnique<CBaseChainParams>("signet", 38332);
    48     else if (chain == CBaseChainParams::REGTEST)
    49         return MakeUnique<CBaseChainParams>("regtest", 18443);
    50-    else
    51-        throw std::runtime_error(strprintf("%s: Unknown chain %s.", __func__, chain));
    52+    throw std::runtime_error(strprintf("%s: Unknown chain %s.", __func__, chain));
    


    jonatack commented at 8:32 am on August 17, 2020:

    6920af6 nit: while here, it would be nice to add brackets to these successive conditionals

    edit: idem in CreateChainParams, line 485

  216. in src/chainparamsbase.cpp:29 in 33b2e22d5a outdated
    23@@ -23,6 +24,9 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman)
    24     argsman.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    25     argsman.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    26     argsman.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    27+    argsman.AddArg("-signet", "Use the signet chain. Note that the network is defined by the signet_blockscript parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    28+    argsman.AddArg("-signet_blockscript", "Blocks must satisfy the given script to be considered valid (only for signet networks)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
    29+    argsman.AddArg("-signet_seednode", "Specify a seed node for the signet network (may be used multiple times to specify multiple seed nodes)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
    


    jonatack commented at 8:34 am on August 17, 2020:
    6920af6 for node operators, perhaps state the accepted format and/or give an example for the seednode and/or blockscript
  217. jonatack commented at 8:38 am on August 17, 2020: member
    Review checkpoint: while building and testing each commit, I’m seeing numerous unit test failures in test/util_tests.cpp at 6920af65. Feel free to ignore the nits below.
  218. in src/signet.cpp:81 in 33b2e22d5a outdated
    75+
    76+    // can't fill any other fields before extracting signet
    77+    // responses from block coinbase tx
    78+
    79+    // find and delete signet signature
    80+    CMutableTransaction cb(*block.vtx.at(0));
    


    practicalswift commented at 9:26 am on August 17, 2020:
    When fuzzing the signet code I noticed that the block.vtx.empty() isn’t handled here. What about handling it by returning invalid, or alternatively asserting that !block.vtx.empty()?

    kallewoof commented at 9:43 am on August 17, 2020:
    Good catch! Done. Also added a commit with your fuzzer (under your name).

    instagibbs commented at 5:36 pm on August 21, 2020:
    s/cb/modified_cb/ ?
  219. practicalswift commented at 9:30 am on August 17, 2020: contributor

    Thanks a lot for your continued work on signet! Really excited about the type of testing this will open up :)

    I wrote a small signet fuzzer which might be worth adding as part of this PR:

     0diff --git a/src/Makefile.test.include b/src/Makefile.test.include
     1index 0068c9407..8eb417cbe 100644
     2--- a/src/Makefile.test.include
     3+++ b/src/Makefile.test.include
     4@@ -134,6 +134,7 @@ FUZZ_TARGETS = \
     5   test/fuzz/scriptnum_ops \
     6   test/fuzz/service_deserialize \
     7   test/fuzz/signature_checker \
     8+  test/fuzz/signet \
     9   test/fuzz/snapshotmetadata_deserialize \
    10   test/fuzz/span \
    11   test/fuzz/spanparsing \
    12@@ -1106,6 +1107,12 @@ test_fuzz_signature_checker_LDADD = $(FUZZ_SUITE_LD_COMMON)
    13 test_fuzz_signature_checker_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
    14 test_fuzz_signature_checker_SOURCES = test/fuzz/signature_checker.cpp
    15
    16+test_fuzz_signet_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
    17+test_fuzz_signet_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
    18+test_fuzz_signet_LDADD = $(FUZZ_SUITE_LD_COMMON)
    19+test_fuzz_signet_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
    20+test_fuzz_signet_SOURCES = test/fuzz/signet.cpp
    21+
    22 test_fuzz_snapshotmetadata_deserialize_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) -DSNAPSHOTMETADATA_DESERIALIZE=1
    23 test_fuzz_snapshotmetadata_deserialize_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
    24 test_fuzz_snapshotmetadata_deserialize_LDADD = $(FUZZ_SUITE_LD_COMMON)
    25diff --git a/src/test/fuzz/signet.cpp b/src/test/fuzz/signet.cpp
    26new file mode 100644
    27index 000000000..5c2979abe
    28--- /dev/null
    29+++ b/src/test/fuzz/signet.cpp
    30@@ -0,0 +1,35 @@
    31+// Copyright (c) 2020 The Bitcoin Core developers
    32+// Distributed under the MIT software license, see the accompanying
    33+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
    34+
    35+#include <chainparams.h>
    36+#include <consensus/validation.h>
    37+#include <primitives/block.h>
    38+#include <signet.h>
    39+#include <streams.h>
    40+#include <test/fuzz/fuzz.h>
    41+#include <test/fuzz/FuzzedDataProvider.h>
    42+#include <test/fuzz/util.h>
    43+
    44+#include <cstdint>
    45+#include <optional>
    46+#include <vector>
    47+
    48+void initialize()
    49+{
    50+    InitializeFuzzingContext(CBaseChainParams::SIGNET);
    51+}
    52+
    53+void test_one_input(const std::vector<uint8_t>& buffer)
    54+{
    55+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
    56+    const std::optional<CBlock> block = ConsumeDeserializable<CBlock>(fuzzed_data_provider);
    57+    if (!block) {
    58+        return;
    59+    }
    60+    (void)CheckBlockSolution(*block, Params().GetConsensus());
    61+    if (GetWitnessCommitmentIndex(*block) != NO_WITNESS_COMMITMENT) {
    62+        (void)SignetTxs(*block, ConsumeScript(fuzzed_data_provider));
    63+    }
    64+}
    
  220. kallewoof commented at 9:44 am on August 17, 2020: member
    @jonatack I won’t rebase/squash anything until you give me the go ahead.
  221. jonatack commented at 9:50 am on August 17, 2020: member

    @kallewoof Thanks, good for now. My only substantive feedback at this point would be to make the commits hygienic by fixing up (or merging the unit test changes into the commits that require them), as the commits starting from 6920af6 fail the unit tests until the last commit. Functional tests to sanity check the conf options handling would be good as well; I’ll propose a commit later today.

    Edit: thanks for the updates, LGTM.

  222. in src/chainparams.cpp:269 in ad33fd5b5f outdated
    269-        }
    270-        const auto blockscript = args.GetArgs("-signet_blockscript");
    271-        if (blockscript.size() != 1) {
    272-            throw std::runtime_error(strprintf("%s: -signet_blockscript cannot be multiple values.", __func__));
    273+            LogPrintf("Using default signet network\n");
    274+            bin = ParseHex("512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae");
    


    fjahr commented at 5:16 pm on August 17, 2020:

    in ad33fd5b5f74bb8ff56ffe901f5a1825e73ecf44

    nit: The help info on the args could be updated to reflect the defaults to the global signet. Right now it’s not obvious that blockscript and seednode are optional.


    fjahr commented at 5:20 pm on August 17, 2020:
    Otherwise all the changes look good to me!
  223. jonatack commented at 6:18 pm on August 17, 2020: member
    Unit tests util_tests still failing starting from 18e18820bb.
  224. kallewoof commented at 3:47 am on August 18, 2020: member
    @jonatack Sorry, forgot about that comment. Checking now.
  225. kallewoof force-pushed on Aug 18, 2020
  226. kallewoof commented at 5:28 am on August 18, 2020: member
    @jonatack Every commit should now compile and unit tests should succeed.
  227. kallewoof force-pushed on Aug 18, 2020
  228. jonatack commented at 7:32 am on August 18, 2020: member

    @jonatack Every commit should now compile and unit tests should succeed.

    Yes, verified all commits at 05115cd. Oops, just rebased again, but looks like no change per git range-diff e6e277f9 05115cd 15a2608.

  229. kallewoof commented at 7:32 am on August 18, 2020: member
    Yeah, sorry, wallet_groups CI failure.
  230. jonatack approved
  231. jonatack commented at 9:05 am on August 18, 2020: member

    Tested ACK 15a26087a34 code review; clean debug build and green unit tests at each commit; fuzz build and ran the new signet fuzzer to 5 million execs; manually retested running signet with the CLI following my steps in #18267 (comment) and verified the bitcoind signet helps and cli errors (e.g. if passing two signet_blockscript args), manually tested running signet with the GUI and ran signet actions with rpc commands in the GUI console. I think this can be merged.

     0$ test/fuzz/signet
     1INFO: Seed: 1520911288
     2INFO: Loaded 1 modules   (328861 inline 8-bit counters): 328861 [0x5567902be7d8, 0x55679030ec75), 
     3INFO: Loaded 1 PC tables (328861 PCs): 328861 [0x55679030ec78,0x556790813648), 
     4INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
     5INFO: A corpus is not provided, starting from an empty corpus
     6[#2](/bitcoin-bitcoin/2/)	INITED cov: 363 ft: 364 corp: 1/1b exec/s: 0 rss: 121Mb
     7.../...
     8[#5079429](/bitcoin-bitcoin/5079429/)	REDUCE cov: 1894 ft: 9844 corp: 317/166Kb lim: 4096 exec/s: 1505 rss: 367Mb L: 177/4096 MS: 2 EraseBytes-InsertRepeatedBytes-
     9[#5081700](/bitcoin-bitcoin/5081700/)	REDUCE cov: 1894 ft: 9844 corp: 317/166Kb lim: 4096 exec/s: 1505 rss: 367Mb L: 8/4096 MS: 1 EraseBytes-
    10[#5082051](/bitcoin-bitcoin/5082051/)	REDUCE cov: 1894 ft: 9844 corp: 317/166Kb lim: 4096 exec/s: 1505 rss: 367Mb L: 220/4096 MS: 1 EraseBytes-
    
  232. michaelfolkson commented at 12:37 pm on August 18, 2020: contributor
    Unit tests, functional tests passing on MacOS Catalina. Received Bitcoin from the Signet faucet, verified my balance locally, sent Signet coins to another address with fee set (Fee estimation failed without fee set)
  233. pinheadmz approved
  234. pinheadmz commented at 10:56 pm on August 18, 2020: member

    Build OK and passed functional and unit tests on OSX 10.14.6 Reviewed all changes as best I could. Signet faucet was down for me – anyone got a coin? sb1q4jtzm222357e5ewrf6gcfzdltm2jdk4efhnuld

    Also tried to -generate and got expected error:

    02020-08-18T22:35:33Z ERROR: CheckBlockSolution: Errors in block (block solution parse failure)
    12020-08-18T22:35:33Z ERROR: ProcessNewBlock: AcceptBlock FAILED (bad-signet-blksig, signet block signature validation failure)
    

    ACK 15a26087a3405903c562fce8c8d7682731b807c8

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA256
     2
     3ACK 15a26087a3405903c562fce8c8d7682731b807c8
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAl88XHkACgkQ5+KYS2KJ
     7yTrgaw/+Pb5Fpa7D8unEoaQ5koxvzhY9PMIvwbe+SA3arAsDVSf/8duHM1o7+ZVD
     8mZgB02tm4JUGWD58MGnCsrKRkXa8MHikilPzAfFUb6IKeFCfJoK9/FUN0zq/w0ve
     9phH26Zvw2KzPfkrZcyDXKOwkvOYXN/ro2a0IhjATMXQRJ26fltCVBiDcahxC4I8g
    10bsDwsCfDx4dDCR6HT/dvrrph+EVZ+nPXYlqdhhXGdYiQvifgvAUr9fTg3dUUL2W5
    11pfZv167yGz3G+5Bgy1kJjTNWLsOKnie881pJIas/Gl//SMm8VBcrYLRxoGICNj6f
    12Kln9rufGyeNJI4RYsRFohmRmIlso9f/LHMTTb3A7qn1l9+gBUaBjOlshNs24kQLt
    136NsOxJsfnkyeeonDcvGHBm2LF/pzIPBRbzN3w5c31p6deYoVtCve/kW66zOs4xa+
    14/u4Ig6MOUCTqv13UP/Oiu85Rhim9hxaI+32LABm6C7MXcIeIoRr1uGHsePr7tnKg
    15C3MzyGzKYqsYXKkDGCu7M7P7Ab40wkBTyQLBSnANtPnu/EK7F2qqWs1sR4VDCiq1
    16NHq2m57LtgYc+iro6WLQS2QQm2SSyXwlXcvWuDuMGDDA+k9/MrqdnQvO7FfHwLoC
    17veCwBsy04NPxt8X4C+SY7GSnqDDWKT9efVUh43IkBwaUwGxTrio=
    18=7d3n
    19-----END PGP SIGNATURE-----
    

    pinheadmz’s public key is on keybase

  235. kallewoof commented at 0:39 am on August 19, 2020: member
    @pinheadmz Sent. (DM me on Twitter with your IP number (or write last half of it or something here, if you don’t mind) and I’ll look.)
  236. pinheadmz commented at 1:44 pm on August 19, 2020: member

    Got some coins from @kallewoof and sent transactions to all 3 address types available from getnewaddress using sendmany.I repeated the send until I got too-long-mempool chain errors. Wallet balances all looked good so all the address types worked.

    Then something kinda interesting:

    02020-08-19T12:58:27Z ERROR: ConnectBlock(): coinbase pays too much (actual=5000113810 vs limit=5000081890)
    12020-08-19T12:58:27Z InvalidChainFound: invalid block=00000899df7024bf4d015a59a89333a3e167e4c9c3667c42a72843a52f3cf751  height=1296  log2_work=28.919402  date=2020-08-19T12:58:25Z
    22020-08-19T12:58:27Z InvalidChainFound:  current best=000021da20b30538459493b10ad54460114d05e4187751c73f26b24889e28201  height=1295  log2_work=28.918289  date=2020-08-19T12:48:21Z
    32020-08-19T12:58:27Z ERROR: ConnectTip: ConnectBlock 00000899df7024bf4d015a59a89333a3e167e4c9c3667c42a72843a52f3cf751 failed, bad-cb-amount
    42020-08-19T12:58:27Z InvalidChainFound: invalid block=00000899df7024bf4d015a59a89333a3e167e4c9c3667c42a72843a52f3cf751  height=1296  log2_work=28.919402  date=2020-08-19T12:58:25Z
    52020-08-19T12:58:27Z InvalidChainFound:  current best=000021da20b30538459493b10ad54460114d05e4187751c73f26b24889e28201  height=1295  log2_work=28.918289  date=2020-08-19T12:48:21Z
    6noticed t2020-08-19T13:24:18Z Potential stale tip detected, will try using extra outbound peer (last tip update: 2153 seconds ago)
    72020-08-19T13:34:48Z Potential stale tip detected, will try using extra outbound peer (last tip update: 2783 seconds ago)
    
  237. kallewoof commented at 1:48 pm on August 19, 2020: member
    @pinheadmz Yep! The block generation script had a cap at 20 txs, but getblocktemplate happily included more, resulting in a block with more subsidy than were accounted for in fees. Good catch, and this has been fixed!
  238. fjahr commented at 1:34 pm on August 21, 2020: member

    tested ACK 15a26087a3405903c562fce8c8d7682731b807c8

    Reviewed changes since my last review per git range-diff a4a279b4f368661ea7d2507dd963469f432f916c..96e3d1e45157cb8f2c7ecae8366fe076cc913742 e6e277f9ed4da7aff9b7b39a7838bada0c3e572a..15a26087a3405903c562fce8c8d7682731b807c8. Changes address feedback and include the new fuzzer.

    Resynced global signet, got some coins from the faucet, and made some test transactions.

  239. in src/signet.cpp:86 in 0855e12485 outdated
    81+    CMutableTransaction cb(*block.vtx.at(0));
    82+
    83+    const int cidx = GetWitnessCommitmentIndex(block);
    84+    assert(cidx != NO_WITNESS_COMMITMENT);
    85+
    86+    CScript& script = cb.vout.at(cidx).scriptPubKey;
    


    instagibbs commented at 4:54 pm on August 21, 2020:
    “script” is really not self-documenting.

    MarcoFalke commented at 3:08 pm on August 22, 2020:

    “script” is really not self-documenting.

    witness_commitment seems a better name

  240. in src/signet.cpp:24 in 0855e12485 outdated
    19+
    20+static constexpr uint8_t SIGNET_HEADER[4] = {0xec, 0xc7, 0xda, 0xa2};
    21+
    22+static constexpr unsigned int BLOCK_SCRIPT_VERIFY_FLAGS = SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_NULLDUMMY;
    23+
    24+static bool ExtractCommitmentSection(CScript& script, const Span<const uint8_t> header, std::vector<uint8_t>& result)
    


    instagibbs commented at 5:03 pm on August 21, 2020:
    s/ExtractCommitmentSection/ExtractCommitmentSectionAndModifyCB/ ?

    instagibbs commented at 5:04 pm on August 21, 2020:
    Could you please re-order the arguments so they are all const first, then non-const, or vice versa?

    instagibbs commented at 5:05 pm on August 21, 2020:
    s/script/witness_commitment_script_to_replace/ or something? really would like this more self-documenting

    kallewoof commented at 5:08 am on August 24, 2020:
    Renamed FetchAndClearCommitmentSection.
  241. in src/signet.cpp:27 in 0855e12485 outdated
    22+static constexpr unsigned int BLOCK_SCRIPT_VERIFY_FLAGS = SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_DERSIG | SCRIPT_VERIFY_NULLDUMMY;
    23+
    24+static bool ExtractCommitmentSection(CScript& script, const Span<const uint8_t> header, std::vector<uint8_t>& result)
    25+{
    26+    CScript replacement;
    27+    bool found = false;
    


    instagibbs commented at 5:10 pm on August 21, 2020:
    s/found/found_header/
  242. in src/signet.cpp:36 in 0855e12485 outdated
    31+    std::vector<uint8_t> pushdata;
    32+    while (script.GetOp(pc, opcode, pushdata)) {
    33+        if (pushdata.size() > 0) {
    34+            if (!found && pushdata.size() > (size_t) header.size() && Span<const uint8_t>(pushdata.data(), header.size()) == header) {
    35+                // pushdata only counts if it has the header _and_ some data
    36+                result.clear();
    


    instagibbs commented at 5:11 pm on August 21, 2020:
    please move this to the top of the function, weird to only clear when successfully finding data
  243. in src/signet.cpp:89 in 0855e12485 outdated
    84+    assert(cidx != NO_WITNESS_COMMITMENT);
    85+
    86+    CScript& script = cb.vout.at(cidx).scriptPubKey;
    87+
    88+    std::vector<uint8_t> data;
    89+    if (!ExtractCommitmentSection(script, SIGNET_HEADER, data)) {
    


    instagibbs commented at 5:37 pm on August 21, 2020:
    s/data/witness_data/

    MarcoFalke commented at 3:13 pm on August 22, 2020:

    s/data/witness_data/

    data includes both the scritptSig and scriptWitness, so instead of witness_data, what about signet_solution (the BIP also calls it that way)

  244. in src/signet.cpp:102 in 0855e12485 outdated
     97+    } catch (const std::exception&) {
     98+        return {tx_to_spend, tx_spending}; // excepted; invalid
     99+    }
    100+    uint256 signet_merkle = ComputeModifiedMerkleRoot(cb, block);
    101+
    102+    data.clear();
    


    instagibbs commented at 5:38 pm on August 21, 2020:
    Just make a new vector named to_spend_data

    MarcoFalke commented at 3:19 pm on August 22, 2020:

    Just make a new vector named to_spend_data

    or block_data (as it is called in the BIP)

  245. instagibbs commented at 5:44 pm on August 21, 2020: member
    Found the BIP’s explanation of the commitment section incomprehensible. Discussing offline, I think everything looks fine otherwise, assuming my reading of the commitment code was what was intended :)
  246. in src/consensus/validation.h:160 in 6aae461a4e outdated
    155+static constexpr int NO_WITNESS_COMMITMENT{-1};
    156+/** Minimum size of a witness commitment structure. Defined in BIP 141. **/
    157+static constexpr size_t MINIMUM_WITNESS_COMMITMENT{38};
    158+
    159+/** Compute at which vout of the given coinbase transaction the witness commitment occurs, or -1 if not found */
    160+static inline int GetWitnessCommitmentIndex(const CBlock& block)
    


    MarcoFalke commented at 1:34 pm on August 22, 2020:

    in commit “validation: move GetWitnessCommitmentIndex to consensus/validation”:

    Any reason to add a static in a header file? Also, why is the docstring changed in a move-only commit?


    kallewoof commented at 5:27 am on August 24, 2020:

    The static inline seems to be useful in some cases, unless I’m misreading https://stackoverflow.com/questions/47819719/static-inline-functions-in-a-header-file . No strong opinion though (and it seems to compile without static).

    Docstring change was a leftover from when I modified this to take a transaction – thanks for pointing it out! Reverting.


    sipa commented at 5:34 am on August 24, 2020:

    static means that if the function gets inlined in every call site, it won’t be materialized as a separate function in the object file. However, if they can’t be inlined, every compilation unit will get its own copy.

    Without static, there will be exactly one copy in the binary, shared by all non-inlined callsites.

    Both will work, but the resulting binary is slightly differently layed out.


    kallewoof commented at 6:54 am on August 24, 2020:
    Makes sense, thank you. Dropping static.
  247. in src/signet.h:17 in 0855e12485 outdated
    12+
    13+#include <array>
    14+#include <cstdint>
    15+#include <vector>
    16+
    17+#include <span.h>
    


    MarcoFalke commented at 2:19 pm on August 22, 2020:

    in commit 0855e12485848b578bd2796d24d5a16cbb763b72:

    Are any of the includes here used in the header? (Except for the first three)


    kallewoof commented at 5:29 am on August 24, 2020:
    They weren’t – moved to signet.cpp.
  248. in src/signet.cpp:124 in 0855e12485 outdated
    119+        return true;
    120+    }
    121+
    122+    int cidx = GetWitnessCommitmentIndex(block);
    123+    if (cidx == NO_WITNESS_COMMITMENT) {
    124+        return error("CheckBlockSolution: Errors in block (no witness commitment)");
    


    MarcoFalke commented at 4:01 pm on August 22, 2020:
    Please don’t use local system errors for remote peer misbehaviour. If you want to log the reject reason, please use a log category (probably VALIDATION?).

    MarcoFalke commented at 4:03 pm on August 22, 2020:
    Also, it seems this check can be removed (if the assert in Create is replaced with a return)

    kallewoof commented at 6:11 am on August 24, 2020:

    E.g. in ReadBlockFromDisk, this isn’t remote peer misbehavior, is it? Either way, it uses error() when errors in the block is encountered, and depends on CheckBlockSolution to do so as well. I could remove that dependency and just put a generic error in ReadBlockFromDisk, but not sure it’s necessary.

    The error() result is unused in CheckBlock().


    MarcoFalke commented at 6:23 am on August 24, 2020:
    What I meant to say is that error() will unconditionally log to disk (something that should happen on local system errors, e.g. ironically out-of-disk). Reject reasons that originate from messages sent by remote peers should be logged with a log category, which can be turned on or off.

    kallewoof commented at 7:00 am on August 24, 2020:

    I’m confused. The other error() calls e.g. in ReadBlockFromDisk are exactly of the same type. If you mean that these are invalid and should (eventually) be fixed, then I’m completely on board.

    The end result, in this case, will still be a log to disk, since ReadBlockFromDisk calls error() – it will just be less detailed/helpful.

    I could break the current pattern, i.e. not call error() on this particular failure type, but every other type does call error(), so it seems sensible. See 1152, 1160, 1165 below.

    https://github.com/bitcoin/bitcoin/blob/a789f251e8fa2d1569c8f4d23f84d75bf74e1f94/src/validation.cpp#L1146-L1174


    sipa commented at 7:05 am on August 24, 2020:

    error() is for system errors, situations where something is wrong with your computer locally, rather than something wrong with the block itself.

    This here looks like a consensus rule violation instead.


    kallewoof commented at 7:13 am on August 24, 2020:
    OK, I think L1164 is similar, but I can sort of get the difference. Changing to logging.

    sipa commented at 3:41 am on August 25, 2020:

    @kallewoof Just to make sure this is clear: L1164 isn’t part of the consensus rules, but just a disk-sanity check.

    The reason is that we don’t store blocks with invalid PoW on disk, as it’s verified before that point is reached (it’s verified in CheckBlockHeader). In ReadBlockFromDisk the check is redundant for that reason, but it’s used as a simple (but rather expensive) checksum that the header is read correctly.

    The code here (I believe?) is the actual signet consensus rule implementation, so it’s more akin to what CheckBlockHeader is doing, and shouldn’t unconditionally log violations.


    kallewoof commented at 5:47 am on August 25, 2020:
    Good point, yep!

    MarcoFalke commented at 6:01 am on August 25, 2020:
    I think this has been correctly solved in the latest force push (Thus I resolved the discussion here)
  249. MarcoFalke commented at 4:32 pm on August 22, 2020: member

    Concept ACK, feel free to ignore the other style-related feedback

    1cc60a05426b02f86f63a2d3f3744ae484e1ac71

  250. in src/signet.cpp:138 in 15a26087a3 outdated
    133+
    134+    const CScript& scriptSig = signet_txs.m_to_sign.vin[0].scriptSig;
    135+    const CScriptWitness& witness = signet_txs.m_to_sign.vin[0].scriptWitness;
    136+
    137+    if (scriptSig.empty() && witness.stack.empty()) {
    138+        return error("CheckBlockSolution: Errors in block (block solution missing)");
    


    MarcoFalke commented at 6:06 pm on August 22, 2020:
    Any reason for this check? Unless I am mistaken, a challenge that allows both to be empty and still be valid also allows the solution to be non-empty and thus mutable. Allowing mutable solutions is probably no less than a footgun that allows empty solutions.

    kallewoof commented at 6:14 am on August 24, 2020:
    Good point; removing this check.
  251. in src/test/util_tests.cpp:852 in 15a26087a3 outdated
    847@@ -848,8 +848,8 @@ struct ArgsMergeTestingSetup : public BasicTestingSetup {
    848             ForEachNoDup(conf_actions, SET, SECTION_NEGATE, [&] {
    849                 for (bool soft_set : {false, true}) {
    850                     for (bool force_set : {false, true}) {
    851-                        for (const std::string& section : {CBaseChainParams::MAIN, CBaseChainParams::TESTNET}) {
    852-                            for (const std::string& network : {CBaseChainParams::MAIN, CBaseChainParams::TESTNET}) {
    853+                        for (const std::string& section : {CBaseChainParams::MAIN, CBaseChainParams::TESTNET, CBaseChainParams::SIGNET}) {
    854+                            for (const std::string& network : {CBaseChainParams::MAIN, CBaseChainParams::TESTNET, CBaseChainParams::SIGNET}) {
    


    MarcoFalke commented at 6:36 pm on August 22, 2020:
    Any reason to add this here? I think the test only wants to test mainnet vs non-mainnet behavior and not specific chain behavior

    kallewoof commented at 6:17 am on August 24, 2020:
    It seems not unreasonable to test signet behavior in this manner as well, so keeping this for now. No strong opinion though, so will remove if you or someone insist.
  252. in src/signet.cpp:19 in 62a618f925 outdated
    16@@ -17,35 +17,43 @@
    17 #include <util/strencodings.h>
    18 #include <util/system.h>
    19 
    


    MarcoFalke commented at 6:26 am on August 24, 2020:
    no need for a new line here

    kallewoof commented at 9:27 am on August 24, 2020:
    Reordered alphabetically and split into ‘C++ stuff’ and ‘Bitcoin Core stuff’.
  253. in src/signet.cpp:136 in a1646829ab outdated
    131+
    132+    int cidx = GetWitnessCommitmentIndex(block);
    133+    if (cidx == NO_WITNESS_COMMITMENT) {
    134+        LogPrint(BCLog::VALIDATION, "CheckBlockSolution: Errors in block (no witness commitment)\n");
    135+        return false;
    136+    }
    


    MarcoFalke commented at 7:42 am on August 24, 2020:
    This check seems duplicate and can be removed. The if (!signet_txs.m_valid) { in the next line already checks for missing witness commitment, missing signet header in the commitment, or incorrect size of the signet_solution

    kallewoof commented at 9:20 am on August 24, 2020:
    Oh, you’re right. Removing.
  254. MarcoFalke commented at 7:42 am on August 24, 2020: member
    Thanks for the fixups. Will re-review after a squash.
  255. in src/signet.cpp:125 in a1646829ab outdated
    120+
    121+    return {tx_to_spend, tx_spending, true};
    122+}
    123+
    124+// Signet block solution checker
    125+bool CheckBlockSolution(const CBlock& block, const Consensus::Params& consensusParams)
    


    MarcoFalke commented at 7:50 am on August 24, 2020:
    nit: Maybe also rename this to CheckSignetBlockSolution to be extra verbose. Also, could add a if (!consensusparams.signet_blocks) return false; or assert(false);

    kallewoof commented at 9:22 am on August 24, 2020:
    Yeah, misleadingly named. Renaming.
  256. MarcoFalke commented at 7:51 am on August 24, 2020: member
    one more nit (feel free to ignore)
  257. kallewoof force-pushed on Aug 24, 2020
  258. kallewoof commented at 9:27 am on August 24, 2020: member
    Addressed @MarcoFalke comments and squashed.
  259. MarcoFalke commented at 9:41 am on August 24, 2020: member

    Approach ACK 2e539c8fce 🔵

    Changes since last review:

    • First commit is a bit more move-only now (–color-moved=dimmed-zebra)
    • Various renamed symbols
    • Removed two checks that appeared redundant
    • Replaced error() with logging to category when checking blocks from p2p. Added error() when checking blocks from local disk

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3Approach ACK 2e539c8fce 🔵
     4
     5Changes since last review:
     6* First commit is a bit more move-only now (--color-moved=dimmed-zebra)
     7* Various renamed symbols
     8* Removed two checks that appeared redundant
     9* Replaced error() with logging to category when checking blocks from p2p. Added error() when checking blocks from local disk
    10-----BEGIN PGP SIGNATURE-----
    11
    12iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    13pUj4jwv/XcMQCGF2ONm0QH/wCSeZBSinuIVIPWHESkmfTl37b7tEnhGoA2reS/KO
    14Q56LPhpKSL6rHDdYZ9UgiVpifGKj3t2tYWpTMELwO9KBFokQQxdrIad9njakBH1N
    15k1lZVx3AcVI4+bcVN6d2ahhrDw4cMhCCiJH+Y4PI8idzifgEVvSKhwt4f2LGzzf4
    162M7i/iXTJQfl3XRyPeGw4+lqOVZeNHlzwYp/PbtPHvK9K087t71P9uQiDA2HKdZ+
    17E3D0WpvkLDnRqjDbWDKAt26sYo0G5Wfh5D7mYv49iABz9c9/e/Hvz9D8lmDC25TK
    18LGZ8s7AA/Yxg0tP9Jc5UE8RpS8t8NAFGuxJ81L6TIXgtw2F87xyKwcQZcIitN+Yp
    19+6T6qTY3t0ZjzePRlD9OMuCFzgMUHVvX2B1g4dH2glT8wNCK2nCROTGXIINWwqNO
    20DcVDY4qi0dQ2VpI6DFOAAJKTIQgtvsqhF1d6aVTEtnzfGw8/1QPR1VsM1kgQH2zi
    21PXjgpqXV
    22=OXBM
    23-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 166d1361fb6b73e01325bfdd8de0862d00829ee56aa1eb658e7709da09c5c947 -

  260. instagibbs commented at 1:27 pm on August 24, 2020: member

    utACK https://github.com/bitcoin/bitcoin/pull/18267/commits/2e539c8fceeed72730528639176a88690731cfd0

    I have some meta-thoughts: Under what circumstances would signet be rebooted? I presume any time there is suspected key theft/loss? Should nets just have a limited lifespan?

  261. michaelfolkson commented at 1:48 pm on August 24, 2020: contributor
    Opened an issue for meta discussion of Signet
  262. fjahr commented at 3:35 pm on August 24, 2020: member

    tested ACK 2e539c8fceeed72730528639176a88690731cfd0

    Reviewed changes since last review per git range-diff e6e277f9ed4da7aff9b7b39a7838bada0c3e572a..15a26087a3405903c562fce8c8d7682731b807c8 e6e277f9ed4da7aff9b7b39a7838bada0c3e572a..2e539c8fceeed72730528639176a88690731cfd0. Resynced global signet and performend test transactions.

    Side-note: This time the faucet txs did not end up in my mempool which is different from my test 3 days ago. I could not find out why that is yet. All other behavior was the same, I could see the txs in the block explorer right away. Did anything change in the faucet node by any chance?

  263. kallewoof commented at 3:15 am on August 25, 2020: member

    @fjahr Thanks for re-review!

    Side-note: This time the faucet txs did not end up in my mempool which is different from my test 3 days ago. I could not find out why that is yet. All other behavior was the same, I could see the txs in the block explorer right away. Did anything change in the faucet node by any chance?

    Weird. It was probably a case of bad connectivity. There are so few nodes right now that you may have not connected to the “right” ones. If it happens again check getconnectioncount and getpeerinfo and see if you see anything odd there?

  264. jonatack commented at 4:02 am on August 25, 2020: member
    re-ACK 2e539c8fceeed727305 reviewed diff since last review per git diff 15a2608 2e539c8, re-reviewed the code in the changed areas, tested running and using signet via the cli.
  265. jonatack commented at 4:07 am on August 25, 2020: member

    Weird. It was probably a case of bad connectivity. There are so few nodes right now that you may have not connected to the “right” ones. If it happens again check getconnectioncount and getpeerinfo and see if you see anything odd there?

    Worked for me just now.

  266. JeremyRubin commented at 5:03 am on August 27, 2020: contributor

    Can you give some instructions for testing this?

    I’m following along with https://en.bitcoin.it/wiki/Signet, but since contrib isn’t in this PR how can I test the block generation stuff?

  267. kallewoof commented at 5:22 am on August 27, 2020: member
    @JeremyRubin This is only the consensus stuff, so it doesn’t have any of the mining related things. I think @ajtowns is opening a PR with the miner stuff soon, if he hasn’t already.
  268. sipa commented at 7:40 pm on August 27, 2020: member

    Perhaps I’m missing some discussion, but I’m not really comfortable with merging this, as it doesn’t seem there are any tests included that exercise the signet code at all.

    Ideally I’d say there should be a functional test that builds a 2-node signet and does some basic interactions. If signing blocks is complicated, it could be something with just a trivial script even, but just something would be far better than nothing.

  269. instagibbs commented at 7:49 pm on August 27, 2020: member
    transaction-style signing should make test-writing with non-OP_TRUE pretty simple I’d hope.
  270. JeremyRubin commented at 7:52 pm on August 27, 2020: contributor
    FWIW I think it’s OK-ish if the tests are in a follow up PR on this exact commit that is opened before this is merged, because people want to bikeshed that part. No need to IMO write brand new code if you have code for launching one already, but it should be possible to test it manually (although automated testing is of course preferred).
  271. kallewoof commented at 1:42 am on August 28, 2020: member
    @sipa It’s a bit of a chicken/egg problem. This PR tries to be minimal and focus on consensus stuff only, which means mining functionality is not present yet. In the original PR (which is referenced in the OP) there are mining tests for signet (although outdated currently).
  272. sipa commented at 1:45 am on August 28, 2020: member
    @kallewoof To be clear: I’m not asking for full support for mining or the ability to build your own signet or anything like that here; just that the code which is introduced in this PR has tests. For example that could just be a slow Python implementation in the test framework that can construct signet blocks for a dummy OP_TRUE challenge script or so…
  273. kallewoof commented at 1:52 am on August 28, 2020: member
    @sipa Got you. I’ll look into that now.
  274. kallewoof force-pushed on Aug 28, 2020
  275. kallewoof force-pushed on Aug 28, 2020
  276. kallewoof force-pushed on Aug 28, 2020
  277. kallewoof force-pushed on Aug 28, 2020
  278. DrahtBot added the label Needs rebase on Aug 28, 2020
  279. kallewoof force-pushed on Aug 28, 2020
  280. kallewoof force-pushed on Aug 28, 2020
  281. kallewoof force-pushed on Aug 28, 2020
  282. kallewoof force-pushed on Aug 28, 2020
  283. kallewoof commented at 4:39 am on August 28, 2020: member

    The test: basic signet tests commit now has a bunch of tests.

    Also changed:

    • chainparams.cpp:
      • gArgsargs
      • The powLimit has been modified to exactly match the compact variant (in genesis) (this is not a hard fork)
    • signet.cpp:
      • An OP_TRUE challenge is now accepted as a valid challenge/solution (used in the first part of the new feature_signet.py tests)
  284. DrahtBot removed the label Needs rebase on Aug 28, 2020
  285. in test/functional/test_framework/test_framework.py:381 in cbb6f9439d outdated
    377@@ -378,7 +378,8 @@ def setup_nodes(self):
    378             for w in wallets[i]:
    379                 wallet_name = w.split('=', 1)[1]
    380                 n.createwallet(wallet_name=wallet_name, descriptors=self.options.descriptors)
    381-        self.import_deterministic_coinbase_privkeys()
    382+        if self.chain != "signet":
    


    MarcoFalke commented at 5:59 am on August 28, 2020:
    why is this needed?

    kallewoof commented at 6:33 am on August 28, 2020:
    Because the privkeys cannot be imported due to signet having a different prefix. (Invalid private key encoding (-5))

    MarcoFalke commented at 6:42 am on August 28, 2020:

    Probably another reason why it would be better to have the same prefix. See also:


    kallewoof commented at 7:03 am on August 28, 2020:
    Switched to using testnet prefixes (including bech32 one).

    kallewoof commented at 7:55 am on August 28, 2020:
    After switching I noticed that rust-bitcoin relies on bech32 prefixes to be unique per network. This change breaks that assumption.

    kallewoof commented at 7:57 am on August 28, 2020:

    It also breaks the assumption that each prefix (pubkey etc) maps to a specific network. Maybe I need to revert this change..

    Edit: it seems to be the case for regtest as well, so I’m patching it up in the hopes that it will “just work”.

    Edit 2: I’ve updated the explorer (which relies on rust-bitcoin) and it seems to be running without problems.

  286. in test/functional/test_framework/test_framework.py:678 in cbb6f9439d outdated
    687-                    nblocks=25 if i != 7 else 24,
    688-                    address=TestNode.PRIV_KEYS[i % 4].address,
    689-                )
    690-
    691-            assert_equal(cache_node.getblockchaininfo()["blocks"], 199)
    692+            if self.chain != "signet":
    


    MarcoFalke commented at 6:00 am on August 28, 2020:
    why is this needed when setup_clean_chain is set to true?

    kallewoof commented at 6:34 am on August 28, 2020:
    Because cache_node is perhaps unable to generate any blocks at all, due to lacking the privkeys to properly sign them.

    MarcoFalke commented at 7:14 am on August 28, 2020:
    locally it is passing for me. What is the error message you get? This code path should only be executed when setup_clean_chain==False

    kallewoof commented at 7:25 am on August 28, 2020:
    Ahh I see what you’re saying. Removing this part, thanks!
  287. in test/functional/feature_signet.py:105 in cbb6f9439d outdated
    100+
    101+        height = 0
    102+        try:
    103+            node.submitblock(signet_blocks[0])
    104+            assert False # this should fail
    105+        except:
    


    MarcoFalke commented at 6:01 am on August 28, 2020:
    Could use assert_raises or a specific exception to avoid catching a keyboard interrupt

    kallewoof commented at 6:38 am on August 28, 2020:
    Actually, that code is broken; the submitblock call does not raise anything, but returns an error string. Fixed though.
  288. in test/functional/feature_signet.py:101 in cbb6f9439d outdated
     96+            assert_equal(node.getblockcount(), height)
     97+
     98+        self.log.info("pregenerated signet blocks check (incompatible solution)")
     99+        node = self.nodes[4]
    100+
    101+        height = 0
    


    MarcoFalke commented at 6:01 am on August 28, 2020:
    why is this set?

    kallewoof commented at 6:39 am on August 28, 2020:
    Left-over, removed. (I wanted to sync and check that peer(s) were at the right heights)
  289. in test/functional/feature_signet.py:28 in cbb6f9439d outdated
    36+        node.generatetoaddress(1, addr)
    37+
    38+class SignetBasicTest(BitcoinTestFramework):
    39+    def set_test_params(self):
    40+        self.chain = "signet"
    41+        self.num_nodes = 6
    


    MarcoFalke commented at 6:03 am on August 28, 2020:
    why 6 nodes when only 3 are queried?

    kallewoof commented at 6:40 am on August 28, 2020:

    I originally intended to pair-wise ensure that the nodes shared blocks, but this was flakey so I removed those checks.

    More importantly though, getblocktemplate requires at least 1 peer or it errors.

    Edit: to clarify, there are 3 separate networks, even though they’re all clumped together as I don’t want to make 3 separate files and classes; nodes 0-1 are on a challenge = OP_TRUE network, nodes 2-3 are on the current default signet (without syncing), nodes 4-5 are on an incompatible signet. Node pairs might disconnect/ban the others, so having 2 for each type seems sensible.

  290. in test/functional/feature_signet.py:87 in cbb6f9439d outdated
    82+        block.nVersion = tmpl["version"]
    83+        block.hashPrevBlock = int(tmpl["previousblockhash"], 16)
    84+        block.nTime = tmpl["curtime"]
    85+        block.nBits = int(tmpl["bits"], 16)
    86+        block.nNonce = 0
    87+        block.vtx = [coinbase_tx]
    


    MarcoFalke commented at 6:07 am on August 28, 2020:
    why create a block that is unused?

    kallewoof commented at 6:42 am on August 28, 2020:
    I think something went missing from the import. The original code does nothing with it either, so removing for now. (Would be good to test a manually crafted block, but I don’t think that’s easy without mining stuff.)
  291. MarcoFalke changes_requested
  292. MarcoFalke commented at 6:12 am on August 28, 2020: member
    Looked at the test, but couldn’t make much sense out of it. Maybe it is better to submit the test/contrib scripts in separate pull requests? This way they can be reviewed and force-pushed as often as needed without dragging the consensus code commits with it every time.
  293. kallewoof force-pushed on Aug 28, 2020
  294. in src/signet.cpp:98 in 800794f52e outdated
    93+
    94+    CScript& witness_commitment = modified_cb.vout.at(cidx).scriptPubKey;
    95+
    96+    std::vector<uint8_t> signet_solution;
    97+    if (!FetchAndClearCommitmentSection(SIGNET_HEADER, witness_commitment, signet_solution)) {
    98+        // no signet solution -- allow this to support OP_TRUE as trivial block challenge
    


    MarcoFalke commented at 6:52 am on August 28, 2020:
    I don’t like this change either. Just because the challenge doesn’t require a signature shouldn’t mean the header can simply be omitted completely

    kallewoof commented at 7:41 am on August 28, 2020:
    It does enable generate* for OP_TRUE signets, though, which is handy.

    MarcoFalke commented at 8:00 am on August 28, 2020:
    Changing consensus code to return early to avoid having to write tests for it seems backward. I’d rather have no tests than a test that requires changing consensus code. Also, this will need to be changed in the BIP first.

    kallewoof commented at 8:37 am on August 28, 2020:

    Honestly, I always assumed that an OP_TRUE challenge with an empty solution was implicitly accepted, even with the BIP as it stands now.

    I read this code as “if it is able to fetch the signet commitment, deserialize it into the scriptSig/witness of the spending tx vin[0]”, i.e. it doesn’t actually return early, it just doesn’t deserialize. The actual validation (i.e. call to VerifyScript(...)) still happens.

    We can remove this an the OP_TRUE test portion for now, though. Thoughts, @ajtowns ?


    ajtowns commented at 8:52 am on August 28, 2020:

    I think having “no block solution” encoded as no signet commitment rather than an empty signet commitment seems slightly better; and it makes it possible for signet with “pushdata(non-zero)” as the challenge act like a unique (as far as magic is concerned), pow-enforcing regtest without needing any additional mining code. I’m not sure that’s useful, but I don’t think it’s worse – you can have an empty challenge to require a signet commitment (with scriptSig “op_true”) if you want. I think the BIP should be changed either way to clarify what happens if the signet commitment isn’t present.

    Also the BIP says the scriptPubKey is called the “block challenge”, but the command line args is “-signet_blockscript”; seems inconsistent?


    kallewoof commented at 9:19 am on August 28, 2020:
    I like challenge/solution, but -signet_blockchallenge looks really long to me…

    MarcoFalke commented at 9:22 am on August 28, 2020:
    The BIP refers to it as signet_challenge, not block challenge?! Am I reading the wrong doc?

    kallewoof commented at 9:33 am on August 28, 2020:

    kallewoof commented at 9:36 am on August 28, 2020:
    Ahh, yeah it does call it signet_challenge in the new version.
  295. kallewoof force-pushed on Aug 28, 2020
  296. in test/functional/feature_signet.py:25 in 567c8813e8 outdated
    20+    '000000204765c5525340783e50e5ab9a60ed45247c0a5376d113ab9027d04524dd260000bfae9d0edb60bcc8314458341a13d5f8d2def60b61abceaf1dda8a03ec7c71c92a1a325f28dc2a1e6d14000001010000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff025851feffffff0200f2052a0100000047512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae0000000000000000766a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf94c4eecc7daa249004730440220179ce76d5f2293a781348eec5ced16ca68d0e12dfda30f169ef8f78952e2242002207a1e848f16019376ada366f00e3d2b48d210bc9f2e2f73c2b82bbd2d170d608d010120000000000000000000000000000000000000000000000000000000000000000000000000',
    21+    '0000002090e20d551753314673ab7b35a4e79276b50b2742acb94d6898f68b8bcb160000cdcb82f191a03ac8d759e33adeaa806d840f1a5aab27d87d62528c834cc8f55d131b325f28dc2a1e791a010001010000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff025951feffffff0200f2052a0100000047512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae0000000000000000766a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf94c4eecc7daa24900473044022052c9fae3f5d4d3e608f71222f15ff6250d70c38877a929a6e3e790b941ca67c1022053f259179e007e184b7fbbd3f0f65b9563f932149f20c4aa772ad6c4343ce59d010120000000000000000000000000000000000000000000000000000000000000000000000000',
    22+    '00000020bbb57e867e82507e4643c6ecdd3c1c185745dbcbf6cd12c72736764ea52a000054f1ff647eef12aa3659d102fccbf9e09f9201bc5372c9e0f29b9e74bec44b29261b325f28dc2a1e9d97000001010000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff025a51feffffff0200f2052a0100000047512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae0000000000000000766a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf94c4eecc7daa24900473044022070264c40d291b035dc734d8faec72302b478869fa6a06e5cf42a3ef11da476b102207bc0817928969ba9aa036739061dfef9edf79ea0638e539e0fd2f30439c2df59010120000000000000000000000000000000000000000000000000000000000000000000000000',
    23+]
    24+
    25+def assert_template(node, block, expect, rehash=True):
    


    MarcoFalke commented at 7:33 am on August 28, 2020:
    unused?
  297. kallewoof force-pushed on Aug 28, 2020
  298. kallewoof force-pushed on Aug 28, 2020
  299. kallewoof commented at 9:45 am on August 28, 2020: member
    Renamed -signet_blockscript to -signet_challenge to match the BIP.
  300. in src/signet.cpp:103 in 6206c2e8e8 outdated
     98+        // no signet solution -- allow this to support OP_TRUE as trivial block challenge
     99+    } else {
    100+        try {
    101+            VectorReader v(SER_NETWORK, INIT_PROTO_VERSION, signet_solution, 0);
    102+            v >> tx_spending.vin[0].scriptSig;
    103+            if (tx_spending.vin[0].scriptSig.empty() && v.empty()) return {tx_to_spend, tx_spending}; // empty commitment; should not be present instead
    


    MarcoFalke commented at 10:24 am on August 29, 2020:
    Again, I don’t understand what this is trying to achieve. See also #18267 (review)

    kallewoof commented at 1:32 pm on August 29, 2020:
    Yeah, I don’t think this is necessary. Removing.
  301. kallewoof force-pushed on Aug 29, 2020
  302. kallewoof commented at 1:39 pm on August 29, 2020: member
    Removed the additional check that the commitment is not empty; we now allow empty commitments for OP_TRUE challenge networks. This has been reflected in the BIP update PR https://github.com/bitcoin/bips/pull/983 as well.
  303. in test/functional/feature_signet.py:31 in 5abbd8f136 outdated
    26+    def generate(self, node, count):
    27+        height = node.getblockcount()
    28+        for _ in range(count):
    29+            addr = node.getnewaddress()
    30+            node.generatetoaddress(1, addr)
    31+        self.wait_until(lambda: node.getblockcount() == height + count, timeout=10)
    


    MarcoFalke commented at 1:59 pm on August 29, 2020:
    why wait? generatetoaddress is blocking, so the assertion can be evaluated immediately after it returns

    kallewoof commented at 10:27 pm on August 30, 2020:
    I saw intermittent failures; you’re right, this is blocking, so this solution is not correct; reverting.
  304. in test/functional/feature_signet.py:26 in 5abbd8f136 outdated
    21+    '0000002090e20d551753314673ab7b35a4e79276b50b2742acb94d6898f68b8bcb160000cdcb82f191a03ac8d759e33adeaa806d840f1a5aab27d87d62528c834cc8f55d131b325f28dc2a1e791a010001010000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff025951feffffff0200f2052a0100000047512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae0000000000000000766a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf94c4eecc7daa24900473044022052c9fae3f5d4d3e608f71222f15ff6250d70c38877a929a6e3e790b941ca67c1022053f259179e007e184b7fbbd3f0f65b9563f932149f20c4aa772ad6c4343ce59d010120000000000000000000000000000000000000000000000000000000000000000000000000',
    22+    '00000020bbb57e867e82507e4643c6ecdd3c1c185745dbcbf6cd12c72736764ea52a000054f1ff647eef12aa3659d102fccbf9e09f9201bc5372c9e0f29b9e74bec44b29261b325f28dc2a1e9d97000001010000000001010000000000000000000000000000000000000000000000000000000000000000ffffffff025a51feffffff0200f2052a0100000047512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae0000000000000000766a24aa21a9ede2f61c3f71d1defd3fa999dfa36953755c690689799962b48bebd836974e8cf94c4eecc7daa24900473044022070264c40d291b035dc734d8faec72302b478869fa6a06e5cf42a3ef11da476b102207bc0817928969ba9aa036739061dfef9edf79ea0638e539e0fd2f30439c2df59010120000000000000000000000000000000000000000000000000000000000000000000000000',
    23+]
    24+
    25+class SignetBasicTest(BitcoinTestFramework):
    26+    def generate(self, node, count):
    


    MarcoFalke commented at 2:00 pm on August 29, 2020:
    why is this even overwritten?

    kallewoof commented at 10:32 pm on August 30, 2020:
    It originally did block signing/grinding and such, but you’re right, it’s not needed at this stage; removing.
  305. in test/functional/feature_signet.py:80 in 5abbd8f136 outdated
    75+            assert_equal(node.submitblock(block), None)
    76+            height = height + 1
    77+            assert_equal(node.getblockcount(), height)
    78+
    79+        self.log.info("pregenerated signet blocks check (incompatible solution)")
    80+        node = self.nodes[4]
    


    MarcoFalke commented at 2:02 pm on August 29, 2020:
    I’d prefer to simply write out the node index. This might even be shorter

    kallewoof commented at 10:33 pm on August 30, 2020:
    That is fine by me.
  306. kallewoof force-pushed on Aug 30, 2020
  307. kallewoof force-pushed on Aug 30, 2020
  308. in src/chainparams.cpp:302 in 6e5e3b3355 outdated
    297+        consensus.nPowTargetSpacing = 10 * 60;
    298+        consensus.fPowAllowMinDifficultyBlocks = false;
    299+        consensus.fPowNoRetargeting = false;
    300+        consensus.nRuleChangeActivationThreshold = 1916;
    301+        consensus.nMinerConfirmationWindow = 2016;
    302+        consensus.powLimit = uint256S("00002adc28000000000000000000000000000000000000000000000000000000");
    


    ajtowns commented at 8:12 am on September 4, 2020:

    math.log2(0x00002adc28000000000000000000000000000000000000000000000000000000 * (14*24*60*60)) gives 257.62... that is in pow.cpp:CalculateNextWorkRequired, if you’re at minimum work and aren’t getting to the next retarget period in less than the target timespan, you’ll overflow the arith_uint256 when trying to work out the next difficulty and get a weird answer. For current signet, that’s resulted in nbits going from 0x1e2adc28 at block 2015 to 0x1e04ffd2 which is a difficulty increase of ~8.5x when it should have been ~0~ 1.07x, and should have been capped at 4x anyway.

    I think we can keep the low difficulty (useful for custom signets, and means we don’t need to get a new genesis block) while avoiding the overflow by changing nPowTargetTimespan to be 1 day instead of 2 weeks.


    kallewoof commented at 1:15 pm on September 6, 2020:
    I would like to avoid deviating from mainnet parameters as much as possible, permanently. It sucks, but another reset may be worth it here.
  309. kallewoof force-pushed on Sep 8, 2020
  310. kallewoof commented at 3:57 am on September 8, 2020: member

    Changes:

    • Skipping length for missing witness data is no longer allowed, as per the BIP changes; this means less corner cases for other software to track
    • Minimum POW difficulty was too low, resulting in overflow at first retargeting; to avoid undefined/unexpected stuff, a decision was made to address this, even though it results in yet another reset (hopefully the last one!)
    • Some clean-up to the signet creation (see https://arne-mertz.de/2016/10/tag-dispatch/ for details on the struct invalid stuff)

    Edit: the explorer and faucet have been updated and now run against the new network.

  311. kallewoof force-pushed on Sep 8, 2020
  312. in src/consensus/validation.h:154 in a25a5e4222 outdated
    150@@ -151,4 +151,30 @@ static inline int64_t GetTransactionInputWeight(const CTxIn& txin)
    151     return ::GetSerializeSize(txin, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * (WITNESS_SCALE_FACTOR - 1) + ::GetSerializeSize(txin, PROTOCOL_VERSION) + ::GetSerializeSize(txin.scriptWitness.stack, PROTOCOL_VERSION);
    152 }
    153 
    154+/** Index marker for when no witness commitment is present in a coinbase transaction. */
    


    jnewbery commented at 4:45 pm on September 9, 2020:
    We usually put constant declarations at the top of the file for visibility. These symbols are included in other translation units.

    kallewoof commented at 1:44 am on September 10, 2020:
    Thanks, moved.
  313. validation: move GetWitnessCommitmentIndex to consensus/validation a2147d7dad
  314. add signet basic support (signet.cpp)
    Co-authored-by: Anthony Towns <aj@erisian.com.au>
    404682b7cd
  315. kallewoof force-pushed on Sep 10, 2020
  316. kallewoof commented at 4:28 am on September 10, 2020: member
    Addressed comment by jnewbery. No changes planned at this point, please re-review so we can get this in!
  317. jonatack approved
  318. jonatack commented at 9:16 am on September 10, 2020: member
    Tested re-ACK dbeea65aec1cc8ff4c3a6ba84950794498fdaae3
  319. michaelfolkson commented at 9:31 am on September 10, 2020: contributor

    ACK dbeea65aec1cc8ff4c3a6ba84950794498fdaae3

    Still seems rough around the edges but for Jon’s (and other’s) sanity probably best we get this merged and then look at follow up PRs.

  320. jonatack commented at 1:27 pm on September 10, 2020: member
    Thanks @michaelfolkson. (If by any chance you are referring to my comment here, I take it back. It was (a) issues with peers still running the previous genesis block code, and (b) I mistakenly blew out my signet conf file while resetting the signet dir.)
  321. michaelfolkson commented at 2:40 pm on September 10, 2020: contributor

    Normally (especially for a consensus change) we would want to get a period of sustained review on a stable iteration, be confident there won’t be any material future changes and I wouldn’t ACK it.

    However, the consensus part is small, it has gone through a number of different iterations, I’m personally broadly happy with the decisions made in the latest iteration, it seems to work fine. There are a number of things I can foresee being changed (e.g. 1-of-2 multisig to a 1-of-3 or 1-of-4) but at what point do we put a stake in the ground and say let’s clean this up with smaller future PRs?

    I’d be interested to hear the concerns of this approach for this specific PR. I understand why this generally shouldn’t be the approach. What do we need to be ACKing? That this is the last iteration, it won’t be reset again, that the consensus part is fine?

    Personally I’d appreciate some guidance on what still needs doing to ensure we can merge this under the understanding that there will need to be further changes to it even if we do. The danger otherwise is that this keeps going in circles with continued changes around the edges. I would be interested to hear thoughts.

  322. fjahr commented at 10:23 pm on September 10, 2020: member

    re-tACK dbeea65aec1cc8ff4c3a6ba84950794498fdaae3

    Code reviewed changes in commits from last review per git range-diff e6e277f9ed4da7aff9b7b39a7838bada0c3e572a..2e539c8fceeed72730528639176a88690731cfd0 15886b08aa5f05194633eba063d7412d0e4fd036..dbeea65aec1cc8ff4c3a6ba84950794498fdaae3 and did fresh review on the new last two test-only commits. Manually tested with a fresh sync and making some transactions.

  323. kallewoof commented at 3:48 am on September 11, 2020: member

    @michaelfolkson

    Normally (especially for a consensus change) we would want to get a period of sustained review on a stable iteration, be confident there won’t be any material future changes and I wouldn’t ACK it.

    While it is a consensus change, it is effectively a “testnet consensus change”, with fairly straightforward logic indicating that it only applies to a signet enabled chain. It’s also been split into its own commit, which is only 11 lines of code (including newlines), so it should be reasonably easy to review.

    There are a number of things I can foresee being changed (e.g. 1-of-2 multisig to a 1-of-3 or 1-of-4) but at what point do we put a stake in the ground and say let’s clean this up with smaller future PRs?

    The default signet network is one thing, and the signet feature itself is another, I think. The current default signet network is a 1-of-2 between me and AJ. God knows what will happen with time, and I think this might surprise all of us. For now, I think it’s a reasonable compromise (if one of us vanishes, the other one can still keep things afloat until we reset or whatnot), and we can decide later on whether to switch to something more sturdy/long-term-viable.

    I’d be interested to hear the concerns of this approach for this specific PR. I understand why this generally shouldn’t be the approach. What do we need to be ACKing? That this is the last iteration, it won’t be reset again, that the consensus part is fine?

    I’m not sure why this shouldn’t be the approach, personally. After all, signet coins are worthless, and we all base the decisions we make on that basic concept – since they’re worthless, it ultimately doesn’t really matter if a single person has all the control, as long as they’re making blocks regularly and not doing any shenanigans (which, again, they have no incentive to do, because the coins are worthless).

    Personally I’d appreciate some guidance on what still needs doing to ensure we can merge this under the understanding that there will need to be further changes to it even if we do. The danger otherwise is that this keeps going in circles with continued changes around the edges. I would be interested to hear thoughts.

    The way I see it is, signet will “start” once we merge the initial version (this), and realize all of the flaws with the initial approach, and then as time goes on, we fine tune it to work as we want it to. A lot of energy has already gone into making this as minimal of an impact as possible, but you can never be sure until you actually start using something actively.

  324. eriknylund commented at 7:17 pm on September 12, 2020: contributor

    tACK dbeea65aec1cc8ff4c3a6ba84950794498fdaae3 built on macOS Catalina 10.15.6 ran tests and functional tests, did not run fuzzer.

    I performed the following tests before RTFM (aka https://gist.github.com/kallewoof/98b6d8dbe126d2b6f47da0ddccd2aa5a):

    Did not configure bitcoin.conf, rather started bitcoind with -signet, so IBD worked but daemon commands not. Synced up to block 2925, which was the tip at that point.

    0./bitcoind -datadir=/tmp/pr18267 -signet
    1getconnectioncount
    2$ ./bitcoin-cli -datadir=/tmp/pr18267 getconnectioncount 
    3error: Could not connect to the server 127.0.0.1:8332
    4
    5Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
    

    Restarted with -chain=signet parameter instead to see if that works, even if help does not mention it among accepted parameters:

     0$ ./bitcoin-cli -h
     1Chain selection options:
     2
     3  -chain=<chain>
     4       Use the chain <chain> (default: main). Allowed values: main, test,
     5       regtest
     6$ ./bitcoind -datadir=/tmp/pr18267 -chain=signet
     72020-09-12T16:32:57Z UpdateTip: new best=0000007a3fbc38ea8934ebcc86eab0bad652aba78d8a75437f697b8cd5dd605b height=2926 version=0x20000000 log2_work=34.398140 tx=2970 date='2020-09-12T16:30:00Z' progress=1.000000 cache=0.0MiB(1txo)
     8$ ./bitcoin-cli -datadir=/tmp/pr18267 getconnectioncount
     9error: Could not connect to the server 127.0.0.1:8332
    10
    11Make sure the bitcoind server is running and that you are connecting to the correct RPC port.
    

    Took the bitcoin.conf route, there seem to be an issue with -chain=signet but -signet works (and of course leaving it out works too):

     0$ mkdir -p  /tmp/St13runtime_error
     1echo "signet=1
     2daemon=1" > /tmp/St13runtime_error/bitcoin.conf
     3$ ./bitcoind -datadir=/tmp/St13runtime_error -chain=signet
     4
     5************************
     6EXCEPTION: St13runtime_error       
     7Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.       
     8bitcoin in AppInit()       
     9
    10Assertion failed: (globalChainBaseParams), function BaseParams, file chainparamsbase.cpp, line 36.
    11Abort trap: 6
    
    0$ cat /tmp/St13runtime_error/bitcoin.conf 
    1signet=1
    2daemon=1
    3$ ./bitcoind -datadir=/tmp/St13runtime_error -signet
    4Bitcoin Core starting
    5$ ./bitcoin-cli -datadir=/tmp/St13runtime_error getconnectioncount
    62
    

    Maybe this behavior is known and expected at this point. Am I wrong to assume/expect that -signet -daemon should work equally well as configuring the values in bitcoin.conf?

    I couldn’t get the signet faucet to work (it keeps giving me Nuh-uh) so I’ll pause my review here until I can acquire some sBTC.

    So far it looks really good. Outstanding work @kallewoof ! 👍

  325. kallewoof commented at 3:27 am on September 13, 2020: member

    @eriknylund Thanks a lot for testing! The faucet will say “Nuh-uh” if you’ve already used it once that day, but maybe something got screwed up. I’ve sent you some coins, though.

    Did not configure bitcoin.conf, rather started bitcoind with -signet, so IBD worked but daemon commands not. Synced up to block 2925, which was the tip at that point.

    The reason is that you did not include -datadir=/tmp/pr18267 -signet in the calls to bitcoin-cli. Since you did not have a config file, you need to provide those for every command.

     0$ mkdir -p  /tmp/St13runtime_error
     1echo "signet=1
     2daemon=1" > /tmp/St13runtime_error/bitcoin.conf
     3$ ./bitcoind -datadir=/tmp/St13runtime_error -chain=signet
     4
     5************************
     6EXCEPTION: St13runtime_error       
     7Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.       
     8bitcoin in AppInit()       
     9
    10Assertion failed: (globalChainBaseParams), function BaseParams, file chainparamsbase.cpp, line 36.
    11Abort trap: 6
    

    Ahh, yeah. This definitely needs fixing; for now, not using -chain should work.

  326. ajtowns commented at 3:49 am on September 13, 2020: member

    there seem to be an issue with -chain=signet Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.

    If you’ve got signet=1 in your conf, specifying -chain=anything on the commandline will error. The assertion failure in addition to the error is pretty lame, but is present in master (bitcoind -testnet -chain=test) too.

  327. eriknylund commented at 7:17 am on September 13, 2020: contributor

    @eriknylund Thanks a lot for testing! The faucet will say “Nuh-uh” if you’ve already used it once that day, but maybe something got screwed up. I’ve sent you some coins, though.

    Thanks for the coins. I think it was a case of PEBCAK. I tried it again today and the faucet worked as intended:

    Payment of 10.00000000 BTC sent with txid 70eb286708b2d680628647599a363495b983bce081af489f73715e2a75c93634

    The reason is that you did not include -datadir=/tmp/pr18267 -signet in the calls to bitcoin-cli. Since you did not have a config file, you need to provide those for every command.

    Thanks for kindly pointing this out. It’s obvious when I see it now. I’ll repeat my steps to make sure this work. Albeit a very small thing, it gave me a deeper insight into how these configurations work behind the scenes.

    I can now see my balance

     0$ ./bitcoin-cli -datadir=/tmp/pr18267 getbalance
     1110.0000000
     2
     3$ ./bitcoin-cli -datadir=/tmp/pr18267 gettransaction 50ea5a5074c282dc63877561d14c2aa8c11c4f00f8b962b21713c3f071a37a29
     4{
     5  "amount": 100.00000000,
     6  "confirmations": 23,
     7  "blockhash": "000000fd523e539ee8741fab5557ca906541ccddb18c8f532e2ebd85d7ffc799",
     8  "blockheight": 2992,
     9  "blockindex": 1,
    10  "blocktime": 1599967800,
    11  "txid": "50ea5a5074c282dc63877561d14c2aa8c11c4f00f8b962b21713c3f071a37a29",
    12  "walletconflicts": [
    13  ],
    14  "time": 1599967826,
    15  "timereceived": 1599967826,
    16  "bip125-replaceable": "no",
    17  "details": [
    18    {
    19      "address": "tb1q542wz3eaexcum4njg7mumqwvag86hw502dkgme",
    20      "category": "receive",
    21      "amount": 100.00000000,
    22      "label": "",
    23      "vout": 0
    24    }
    25  ],
    26  "hex": "02000000000103f88072817493b332cee5647a6ef06b50cd3040353a7ef2d0a1a7f79697ca84530000000000feffffffcff070331e946a95ae4d2120bce7efa63c0d1d6984646a9096b95209b63a16240000000000feffffffa2c9556ce9e69f839151b43303036ea89bbea16f76720720c6cefa1839012fe50000000000feffffff0200e40b5402000000160014a554e1473dc9b1cdd67247b7cd81ccea0fabba8fecf0052a01000000160014232d82ad2dd367e4672169d336220f29b5966f6d0247304402204655e573c4a016f3d159da2b6a35e30f1d0fb0e29baf4634bfceaf9f4809f998022074a4967177d76066d25c29bfc38e005a7d3e2bf1fdaf33274eaa9d75a9b1cdcb01210375c1648db9f764e8c55ed5185d6c0c858a1c1d34a9e3c0b1ad74f8b395bd643b0247304402205ace84abfac338bd19488acf1c7265358a8c5291671c74937195b605e706bf0f0220135c9cf40132e1961368c890302b8f255f74547de662d9d787badb16f4c0f38101210375c1648db9f764e8c55ed5185d6c0c858a1c1d34a9e3c0b1ad74f8b395bd643b02473044022079e573339bfe2ef51c38fc88fc662d511584d9a278c25583e5c0211f76ce30f402204441d6796ccd9c48ff36f5ebd13edc55a087b3144d2950f800856b2dfc5dbfb401210375c1648db9f764e8c55ed5185d6c0c858a1c1d34a9e3c0b1ad74f8b395bd643baf0b0000"
    27}
    28
    29$ ./bitcoin-cli -datadir=/tmp/pr18267 gettransaction 70eb286708b2d680628647599a363495b983bce081af489f73715e2a75c93634
    30{
    31  "amount": 10.00000000,
    32  "confirmations": 1,
    33  "blockhash": "00000080285a6fd52632f17814ee6525429170b6e0a508e73c80e4402494fe50",
    34  "blockheight": 3014,
    35  "blockindex": 1,
    36  "blocktime": 1599981000,
    37  "txid": "70eb286708b2d680628647599a363495b983bce081af489f73715e2a75c93634",
    38  "walletconflicts": [
    39  ],
    40  "time": 1599980676,
    41  "timereceived": 1599980676,
    42  "bip125-replaceable": "no",
    43  "details": [
    44    {
    45      "address": "tb1qyxatme4ggsxjsymle0k53csel5tleqfgcu8m9t",
    46      "category": "receive",
    47      "amount": 10.00000000,
    48      "label": "",
    49      "vout": 0
    50    }
    51  ],
    52  "hex": "020000000001010fbd483e1ec45cd2925f71209db315ab60106f3f9d509b9e2520d6da70dc905c0000000000feffffff0200ca9a3b0000000016001421babde6a8440d28137fcbed48e219fd17fc8128eafde191e600000016001428e5662a809bb4e7d5e1b192d09cb12fa42bfe2a024730440220782359408c80c4600c1c5ad451b2a5e22090b647cfcfe77bf3b1f0c558610ef402202a884638cc7b26e84f2624606314d02e7c08ea0e21dd2de0a9941798807ee852012103e53ae50c6ecfdc66a94e02208743923ebf9ac20f62106924d78172a0a45528c2c50b0000"
    53}
    

    I then proceeded to send some coins over to a second wallet and that worked well. I first received an error about the fee estimation, but that is perhaps to be expected.

     0$ ./bitcoin-cli -datadir=/tmp/pr18267 sendtoaddress "tb1qccu5k3l2fpc5fphmd75a3llfqlsw9msrh956nh" 2.89985002 "signet send test for [#18267](/bitcoin-bitcoin/18267/)"
     1error code: -6
     2error message:
     3Fee estimation failed. Fallbackfee is disabled. Wait a few blocks or enable -fallbackfee.
     4
     5$ ./bitcoin-cli -datadir=/tmp/pr18267 sendtoaddress "tb1qccu5k3l2fpc5fphmd75a3llfqlsw9msrh956nh" 2.89985002 "signet send test for [#18267](/bitcoin-bitcoin/18267/)" "" false true 1 sat/B
     6821672c87a723ee7053a8d6ee513bb6632a5d1ba7434ac2866cb2954abee3006
     7
     8$ ./bitcoin-cli -datadir=/tmp/pr18267 gettransaction 821672c87a723ee7053a8d6ee513bb6632a5d1ba7434ac2866cb2954abee3006
     9{
    10  "amount": -2.89985002,
    11  "fee": -0.00000141,
    12  "confirmations": 1,
    13  "blockhash": "00000033631f0508ab6211af734aabca6ee746dc67d63553ec6d8682af55663f",
    14  "blockheight": 3018,
    15  "blockindex": 1,
    16  "blocktime": 1599983400,
    17  "txid": "821672c87a723ee7053a8d6ee513bb6632a5d1ba7434ac2866cb2954abee3006",
    18  "walletconflicts": [
    19  ],
    20  "time": 1599983335,
    21  "timereceived": 1599983335,
    22  "bip125-replaceable": "no",
    23  "comment": "signet send test for [#18267](/bitcoin-bitcoin/18267/)",
    24  "details": [
    25    {
    26      "address": "tb1qccu5k3l2fpc5fphmd75a3llfqlsw9msrh956nh",
    27      "category": "send",
    28      "amount": -2.89985002,
    29      "vout": 0,
    30      "fee": -0.00000141,
    31      "abandoned": false
    32    }
    33  ],
    34  "hex": "02000000000101297aa371f0c31317b262b9f8004f1cc1a82a4cd161758763dc82c274505aea500000000000fdffffff02ead1481100000000160014c6394b47ea48714486fb6fa9d8ffe907e0e2ee038911c342020000001600143e9c78a55f49f39e6ac3d732a539872a4ff1a7950247304402202afc9a1c181c09f4d157e05bce12761b0594ea05e682e745efcc5c6d36203d8302205e9d268d01d167693abf44f5cf84531506c65feda07693f39657a06de39348db012103e05ec20667b5200e1431fe548e34cb91b81b6a25850ce6a5721df4ff5ed6fe26c90b0000"
    35}
    
  328. eriknylund commented at 8:20 am on September 13, 2020: contributor

    I redid my tests per instructions from @kallewoof and @ajtowns and can confirm it works as I initially imagined to run it - with bitcoind in the foreground and then running bitcoin-cli with the -signet argument in a separate window thus skipping the bitcoin.conf step:

    0mdkir -p /tmp/cli-args-only
    1./bitcoind -datadir=/tmp/cli-args-only -signet
    22020-09-13T08:10:16Z UpdateTip: new best=0000002c6d162d115a91ed5759e22344f51d479dd1ef1f3a3eada7dee1473edc height=3020 version=0x20000000 log2_work=34.480471 tx=3067 date='2020-09-13T08:10:00Z' progress=1.000000 cache=0.0MiB(1txo)
    
    0$ ./bitcoin-cli -datadir=/tmp/cli-args-only -signet getconnectioncount
    11
    2$ ./bitcoin-cli -datadir=/tmp/cli-args-only -signet getblockcount
    33020
    
  329. kallewoof commented at 2:33 pm on September 13, 2020: member
    @MarcoFalke You have a change-request state right now. Mind re-reviewing?
  330. in src/validation.cpp:1167 in fba375689b outdated
    1163@@ -1163,6 +1164,11 @@ bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::P
    1164     if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
    1165         return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
    1166 
    1167+    // Signet only: check block solution
    


    sipa commented at 7:33 pm on September 13, 2020:

    In commit “consensus: add signet validation”:

    Is this necessary? If a block is corrupted on disk, the PoW check above will already catch it.


    kallewoof commented at 4:28 am on September 14, 2020:
    That assumes that the signet validation has been done for all blocks that are written to disk, which may be the case now, but perhaps not always.
  331. in src/signet.cpp:98 in 404682b7cd outdated
    93+
    94+    CScript& witness_commitment = modified_cb.vout.at(cidx).scriptPubKey;
    95+
    96+    std::vector<uint8_t> signet_solution;
    97+    if (!FetchAndClearCommitmentSection(SIGNET_HEADER, witness_commitment, signet_solution)) {
    98+        // no signet solution -- allow this to support OP_TRUE as trivial block challenge
    


    sipa commented at 8:09 pm on September 13, 2020:

    In commit “add signet basic support (signet.cpp)”:

    It looks like this is actually just implying a spending transaction with empty scriptSig/witness stack, not one that contains OP_TRUE. Is the comment outdated?


    ajtowns commented at 11:21 pm on September 13, 2020:

    Your observation is accurate; it’s not outdated per se, it was always just a non-exclusive list of things allowed… (this is why I don’t write comments :)

    Another interesting challenge is something like DEPTH IF DROP ENDIF DEPTH IF DROP ENDIF DEPTH IF DROP ENDIF TRUE which (ignoring magic) would follow the most work chain from any signet whose block solutions weren’t witness based and contained 0-3 stack items.

  332. in src/chainparamsbase.cpp:28 in de1a5d1bba outdated
    23@@ -23,6 +24,9 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman)
    24     argsman.AddArg("-segwitheight=<n>", "Set the activation height of segwit. -1 to disable. (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
    25     argsman.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    26     argsman.AddArg("-vbparams=deployment:start:end", "Use given start/end times for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
    27+    argsman.AddArg("-signet", "Use the signet chain. Note that the network is defined by the signet_challenge parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
    28+    argsman.AddArg("-signet_challenge", "Blocks must satisfy the given script to be considered valid (only for signet networks; defaults to the global default signet test network challenge)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
    


    sipa commented at 8:12 pm on September 13, 2020:

    In commit “add signet chain and accompanying parameters”:

    There doesn’t seem to be a default signet test network challenge in this commit.


    kallewoof commented at 4:26 am on September 14, 2020:
    Edit: I see what you’re saying now. Yeah, it’s chronologically problematic. If I end up rebasing I will tweak this to appear in the commit along with the defaults.

    laanwj commented at 4:49 pm on September 15, 2020:
    BTW: this is the first command line argument with _, or any kind of separator—for that matter— in it. We might want to discuss what we like to see here. Personally, I’d expect -signet-challenge. Or just -signetchallenge to be consistent with say, -segwitheight.

    sipa commented at 4:55 pm on September 15, 2020:
    Very vaguely related, I just saw this: https://twitter.com/koenvervloesem/status/1305911670083067904

    kallewoof commented at 5:26 am on September 16, 2020:
    I don’t have a strong opinion on it, other than it feels like a good idea to clearly separate signet stuff from the rest. I believe we will have several more parameters in the future, e.g. for selecting soft forks or to define alternative challenges (opt-in reorg chain, if we end up going that route).

    laanwj commented at 6:44 pm on September 17, 2020:
    Sure, agree on that, but starting the options with signet should already be enough to keep them separate from the rest.

    kallewoof commented at 0:37 am on September 18, 2020:
    Switching to -signet<suffix>.

    kallewoof commented at 1:20 am on September 18, 2020:
    Also addressed original note that there is no default signet at this point; this line is now modified in the “hard coded default signet” commit to add in the note about default network.
  333. meshcollider requested review from MarcoFalke on Sep 16, 2020
  334. add signet chain and accompanying parameters
    Co-authored-by: Anthony Towns <aj@erisian.com.au>
    e8990f1214
  335. consensus: add signet validation a8de47a1c9
  336. qt: update QT to support signet network c7898bca4e
  337. kallewoof force-pushed on Sep 18, 2020
  338. signet: hard-coded parameters for Signet Global Network VI (2020-09-07)
    Co-authored-by: Anthony Towns <aj@erisian.com.au>
    3efe298dcc
  339. test: signet network selection tests ec9b25d046
  340. test: add small signet fuzzer 4c189abdc4
  341. test: basic signet tests e47ad375bf
  342. test: some sanity checks for consensus logic 8258c4c007
  343. kallewoof force-pushed on Sep 18, 2020
  344. kallewoof commented at 1:22 am on September 18, 2020: member
    Updated to rename -signet_* to -signet*.
  345. kallewoof commented at 7:03 am on September 18, 2020: member
    I have no idea why feature_pruning is failing in that one test in Travis. It works on my mac and linux machines.
  346. jonatack commented at 9:45 am on September 18, 2020: member

    I have no idea why feature_pruning is failing in that one test in Travis. It works on my mac and linux machines.

    I think the CI job failure Method not found (wallet method is disabled because no wallet is loaded) (-32601) is from #15454 merged yesterday.

  347. jonatack commented at 9:47 am on September 18, 2020: member

    re-ACK 8258c4c0076bb5f27efdc117a04b27fcd6dd00b per git diff dbeea65 8258c4c, only change since last review is updated -signet* config option naming.

    Updated config options and help:

     0((HEAD detached from origin/pr/18267))$ ./src/bitcoind -signet -help | grep -A2 signet
     1  -signet
     2       Use the signet chain. Note that the network is defined by the
     3       -signetchallenge parameter
     4
     5  -signetchallenge
     6       Blocks must satisfy the given script to be considered valid (only for
     7       signet networks; defaults to the global default signet test
     8       network challenge)
     9
    10  -signetseednode
    11       Specify a seed node for the signet network, in the hostname[:port]
    12       format, e.g. sig.net:1234 (may be used multiple times to specify
    13       multiple seed nodes; defaults to the global default signet test
    14       network seed node(s))
    
     0$ ./src/bitcoin-cli -datadir=src/signet -netinfo 4
     1Bitcoin Core v0.20.99.0-178345497c - 70016/Satoshi:0.20.99/
     2
     3Peer connections sorted by direction and min ping
     4<-> relay   net mping   ping send recv  txn  blk uptime id address                      version
     5out  full onion   477    477   40   40         5      6  0 vpytsykcl4w4dr2x.onion:38333 70016/Satoshi:0.20.99/
     6                   ms     ms  sec  sec  min  min    min
     7
     8        ipv4    ipv6   onion   total  block-relay
     9in         0       0       0       0       0
    10out        0       0       1       1       0
    11total      0       0       1       1       0
    12
    13Local addresses
    14nhvodudwrn5pka7l.onion                    port 38333     score      4
    15ugv7httwbjaayfuj.onion                    port 38333     score      4
    16
    17$ ./src/bitcoin-cli -datadir=src/signet -getinfo
    18{
    19  "version": 209900,
    20  "blocks": 3747,
    21  "headers": 3747,
    22  "verificationprogress": 1,
    23  "timeoffset": 0,
    24  "connections": {
    25    "in": 0,
    26    "out": 1,
    27    "total": 1
    28  },
    29  "proxy": "127.0.0.1:9050",
    30  "difficulty": 0.003293189602495658,
    31  "chain": "signet",
    32  "keypoolsize": 1000,
    33  "paytxfee": 0.00000000,
    34  "balance": 9.49818000,
    35  "relayfee": 0.00001000,
    36  "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
    37}
    38
    39$ ./src/bitcoin-cli -datadir=src/signet getwalletinfo
    40{
    41  "walletname": "",
    42  "walletversion": 169900,
    43  "balance": 9.49818000,
    44  "unconfirmed_balance": 0.00000000,
    45  "immature_balance": 0.00000000,
    46  "txcount": 10,
    47  "keypoololdest": 1599664406,
    48  "keypoolsize": 1000,
    49  "hdseedid": "xxx",
    50  "keypoolsize_hd_internal": 1000,
    51  "paytxfee": 0.00000000,
    52  "private_keys_enabled": true,
    53  "avoid_reuse": false,
    54  "scanning": false,
    55  "descriptors": false
    56}
    

    Screenshot from 2020-09-18 11-37-42

  348. fjahr commented at 10:45 am on September 18, 2020: member

    re-ACK 8258c4c

    The only change was the renaming of the -signet* args.

  349. in src/chainparams.cpp:332 in 8258c4c007
    327+        base58Prefixes[SCRIPT_ADDRESS] = std::vector<unsigned char>(1,196);
    328+        base58Prefixes[SECRET_KEY] =     std::vector<unsigned char>(1,239);
    329+        base58Prefixes[EXT_PUBLIC_KEY] = {0x04, 0x35, 0x87, 0xCF};
    330+        base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94};
    331+
    332+        bech32_hrp = "tb";
    


    dr-orlovsky commented at 7:31 pm on September 18, 2020:

    Why we do use “tb” instead of “sb” here?

    In c-lightning PR we had “sb” here: https://github.com/ElementsProject/lightning/pull/2816/files#diff-53092192708008b1de0d997128c58d37R54

    And BIP-325 and BIP-173 are being silent in this regard: not even a PR


    kallewoof commented at 8:49 am on September 19, 2020:
    People thinks that the test networks should use the same prefixes so it was changed to match testnet.

    dr-orlovsky commented at 9:26 am on September 19, 2020:

    Alright, probably we have another thing to update in c-lightning then, additionally to new nonce.

    Also, noticed that BIP-235 still has outdated nonce info


    ajtowns commented at 6:17 am on September 21, 2020:
    Filed https://github.com/bitcoin/bips/pull/1000 to update the bip for the genesis block. FWIW, #12314 has related discussion about reusing testnet addresses in regtest.

    kallewoof commented at 9:51 am on September 21, 2020:
    Thanks for pointing that out, @dr-orlovsky – I thought I had already updated the BIP but I had apparently forgotten to actually make it into a pull request.
  350. dr-orlovsky changes_requested
  351. dr-orlovsky commented at 7:32 pm on September 18, 2020: none
    Seems like bech32 HRP is not updated
  352. kallewoof commented at 8:49 am on September 19, 2020: member
    Travis now passes after wallet PR fix. Please re-review.
  353. in src/signet.cpp:134 in 404682b7cd outdated
    129+    }
    130+
    131+    const CScript challenge(consensusParams.signet_challenge.begin(), consensusParams.signet_challenge.end());
    132+    const SignetTxs signet_txs(block, challenge);
    133+
    134+    if (!signet_txs.m_valid) {
    


    MarcoFalke commented at 11:03 am on September 20, 2020:
    might be good to rename m_valid to has_txs or similar, since the sigcheck validation hasn’t been run at this point, so it is unknown whether the txs are valid.

    ajtowns commented at 6:23 am on September 21, 2020:
    Could invert it and call it m_parse_failure. Could even add bool operator!() const { return m_parse_failure; } so the test is just if (!signet_txs). Seems very nit-y though.

    kallewoof commented at 9:49 am on September 21, 2020:
    I think this is nit-ty enough that it’s worth leaving as is, if it means retaining tACKs.

    MarcoFalke commented at 2:03 pm on September 22, 2020:
    Fixed in #19993
  354. in src/test/fuzz/signet.cpp:31 in 4c189abdc4 outdated
    26+    const std::optional<CBlock> block = ConsumeDeserializable<CBlock>(fuzzed_data_provider);
    27+    if (!block) {
    28+        return;
    29+    }
    30+    (void)CheckSignetBlockSolution(*block, Params().GetConsensus());
    31+    if (GetWitnessCommitmentIndex(*block) != NO_WITNESS_COMMITMENT) {
    


    MarcoFalke commented at 11:14 am on September 20, 2020:
    why is this check needed? This will limit coverage

    kallewoof commented at 9:46 am on September 21, 2020:

    MarcoFalke commented at 2:03 pm on September 22, 2020:
    Fixed in #19993
  355. in test/functional/feature_signet.py:56 in e47ad375bf outdated
    51+        assert 'currentblocktx' not in mining_info
    52+        assert 'currentblockweight' not in mining_info
    53+        assert_equal(mining_info['networkhashps'], Decimal('0'))
    54+        assert_equal(mining_info['pooledtx'], 0)
    55+
    56+        self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())
    


    MarcoFalke commented at 11:16 am on September 20, 2020:

    How is this different from just

    0        self.nodes[0].generate(1)
    

    Ideally, we could run the tests without wallet


    kallewoof commented at 9:47 am on September 21, 2020:
    I think this can wait until after merge, unless I need to rebase again.

    MarcoFalke commented at 2:03 pm on September 22, 2020:
    Agree this was non-blocking. Fixed in #19993
  356. in test/functional/feature_signet.py:63 in e47ad375bf outdated
    58+        self.log.info("pregenerated signet blocks check")
    59+
    60+        height = 0
    61+        for block in signet_blocks:
    62+            assert_equal(self.nodes[2].submitblock(block), None)
    63+            height = height + 1
    


    MarcoFalke commented at 11:17 am on September 20, 2020:

    style nit

    0            height += 1
    

    kallewoof commented at 9:47 am on September 21, 2020:
    I think this can wait too; will do if I end up requiring a rebase.

    MarcoFalke commented at 2:03 pm on September 22, 2020:
    Agree the style nit was non-blocking. Fixed in #19993
  357. MarcoFalke approved
  358. MarcoFalke commented at 11:25 am on September 20, 2020: member

    Approach ACK 8258c4c007 🌵

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3Approach ACK 8258c4c007 🌵
     4-----BEGIN PGP SIGNATURE-----
     5
     6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
     7pUhgkAv9HshmtvJ5n8HJ+i7B/2ZZCH5k4QsVC7dOG62A9TJf25LL1vSbmQLMNcmr
     8rYihCi+Hl9gtMLj4ond8EgYrXTRLxcwwaiJIBrnpb/5dJnIH5W65HG3rFN6fBzSg
     9Sf+DmYi990uJUcB9/TSmofpAff4ufc5+1alnXaDDAeo9fMcDgjs79sWy28eGClw3
    10nP5JSU8iJd8u6dQsKmqUmjvP/BTSA+biz3Uz9Q0WLxeYFuX1p3rryo6IfBTZx7iB
    11/aQFJgukOjaQSVDOlKUfVGkAX3EBA5tcNhPU4eQ6sJORKQhopuHvq1JrunWJ7lSz
    12hjiIDw2vI6mEcC7N2zfVCXZu5OV84EWA5GYK4uC44j5dulPbwIEPEMbhSXAi1TLO
    13lY1Lve7ot5/YVGW2TqVa5JhJxv6CdH7Drj8QPQQ00YwUknlUQjiXtT/sB/W6VEZP
    14/4wB9KdOaO+d/e8xq+K5WaGnk7dDiuOXwiSKyKXyxbve9mSIXSxwgBNM6pf6GUvt
    15dS9yZ6ox
    16=IKXe
    17-----END PGP SIGNATURE-----
    

    Timestamp of file with hash 46dca4399a20a8d06fcc6a4bb3621364ceb7aa953eaaeec52880e8248a1abbbd -

  359. laanwj commented at 8:31 pm on September 21, 2020: member
    ACK 8258c4c0076bb5f27efdc117a04b27fcd6dd00b2
  360. laanwj merged this on Sep 21, 2020
  361. laanwj closed this on Sep 21, 2020

  362. sidhujag referenced this in commit dc85269389 on Sep 22, 2020
  363. fanquake removed this from the "Blockers" column in a project

  364. in src/chainparams.cpp:272 in 8258c4c007
    267+        if (!args.IsArgSet("-signetchallenge")) {
    268+            LogPrintf("Using default signet network\n");
    269+            bin = ParseHex("512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430210359ef5021964fe22d6f8e05b2463c9540ce96883fe3b278760f048f5189f2e6c452ae");
    270+            vSeeds.emplace_back("178.128.221.177");
    271+            vSeeds.emplace_back("2a01:7c8:d005:390::5");
    272+            vSeeds.emplace_back("ntv3mtqw5wt63red.onion:38333");
    


    Sjors commented at 10:19 am on September 22, 2020:
    It’s a bit weird that this shows up as “Loading addresses from DNS seed”, but that’s out of scope for this PR.
  365. MarcoFalke referenced this in commit 8219893825 on Sep 23, 2020
  366. sidhujag referenced this in commit edce812404 on Sep 23, 2020
  367. domob1812 referenced this in commit 0e77b03aba on Sep 28, 2020
  368. guggero referenced this in commit 2f74446f17 on Feb 14, 2021
  369. guggero referenced this in commit d9b1cad06f on Feb 17, 2021
  370. guggero referenced this in commit 72cba19f19 on Mar 10, 2021
  371. guggero referenced this in commit 91a21e38d5 on Mar 15, 2021
  372. guggero referenced this in commit 73ecb5997b on Apr 22, 2021
  373. marni referenced this in commit 950e8b3fd1 on Oct 5, 2021
  374. roylee17 referenced this in commit d17e62ae3e on Oct 20, 2021
  375. kcalvinalvin referenced this in commit fd711cec22 on Nov 17, 2021
  376. DrahtBot locked this on Feb 15, 2022
  377. kallewoof deleted the branch on May 16, 2022

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-07-01 13:12 UTC

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