BIP-325: Signet [consensus] #18267

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)

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

Concept ACK

Concept ACK. Local build and tests green, will review shortly.

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

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

oops

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... :)

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..

Signet worked perfectly the first try :100: ...following the instructions at https://gist.github.com/kallewoof/98b6d8dbe126d2b6f47da0ddccd2aa5a... now reviewing

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

<details> <summary>Here is what I did after building this PR branch... :memo:</summary> <p>

cd src
mkdir signet
echo "signet=1" > signet/bitcoin.conf

./bitcoind -datadir=signet  # in a separate terminal buffer to watch the debug log

./bitcoin-cli -datadir=signet -getinfo

./bitcoin-cli -datadir=signet getnewaddress
sb1q77gyzqu9fawygs9suvyfaga2f4mn9kmz2yudk0

./bitcoin-cli -datadir=signet setlabel sb1q77gyzqu9fawygs9suvyfaga2f4mn9kmz2yudk0 testing-signet

./bitcoin-cli -datadir=signet getaddressinfo sb1q77gyzqu9fawygs9suvyfaga2f4mn9kmz2yudk0
{
  "address": "sb1q77gyzqu9fawygs9suvyfaga2f4mn9kmz2yudk0",
  "scriptPubKey": "0014f7904103854f5c4440b0e3089ea3aa4d7732db62",
  "ismine": true,
  "solvable": true,
  "desc": "wpkh([6352002d/0'/0'/0']028f681e25caadc71f45e7811775f4e73b5b27ce7bf335888d514a9fc3999c55c1)#de6xjlkq",
  "iswatchonly": false,
  "isscript": false,
  "iswitness": true,
  "witness_version": 0,
  "witness_program": "f7904103854f5c4440b0e3089ea3aa4d7732db62",
  "pubkey": "028f681e25caadc71f45e7811775f4e73b5b27ce7bf335888d514a9fc3999c55c1",
  "ischange": false,
  "timestamp": 1588011190,
  "hdkeypath": "m/0'/0'/0'",
  "hdseedid": "50a32a27b26929927079cef87b19a7ffb673871b",
  "hdmasterfingerprint": "6352002d",
  "labels": [
    "testing-signet"
  ]
}

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

Payment of 10.00000000 BTC sent with txid 1fb49b9b484e42f5f19a1e5491b1760f949c303a297a63b68e41e9e9f379b3d5

Verify reception

./bitcoin-cli -datadir=signet gettransaction 1fb49b9b484e42f5f19a1e5491b1760f949c303a297a63b68e41e9e9f379b3d5
{
  "amount": 10.00000000,
  "confirmations": 0,
  "trusted": false,
  "txid": "1fb49b9b484e42f5f19a1e5491b1760f949c303a297a63b68e41e9e9f379b3d5",
  "walletconflicts": [
  ],
  "time": 1588011612,
  "timereceived": 1588011612,
  "bip125-replaceable": "no",
  "details": [
    {
      "address": "sb1q77gyzqu9fawygs9suvyfaga2f4mn9kmz2yudk0",
      "category": "receive",
      "amount": 10.00000000,
      "label": "testing-signet",
      "vout": 0
    }
  ],
  "hex": "0200000000010154c0f4e926d77105e30311dd126f3a06aed63f7aa722702dd0c606e2f05a8db80100000000feffffff0200ca9a3b00000000160014f7904103854f5c4440b0e3089ea3aa4d7732db62fc33c6b8000000001600149218f528e0ebd953fdc72a7f42150facbe545ee1024730440220422cd8b64cdd49a87636ada6d41bfa761b592f99edc7d21081ba2b54e4d0672502207156fba2cec09d990a243c6925f4bca53e123730d13076d3e68cc0f96795e645012102a53a66b296489e8cd996c216d2c90e682187cce883f55d06c188daf5e8779816101f0000"
}

./bitcoin-cli -datadir=signet getbalances
{
  "mine": {
    "trusted": 0.00000000,
    "untrusted_pending": 10.00000000,
    "immature": 0.00000000
  }
}

...and after a few minutes...

./bitcoin-cli -datadir=signet getbalances
{
  "mine": {
    "trusted": 10.00000000,
    "untrusted_pending": 0.00000000,
    "immature": 0.00000000
  }
}

./bitcoin-cli -datadir=signet -getinfo
{
  "version": 209900,
  "blocks": 8055,
  "headers": 8055,
  "verificationprogress": 1.544713757023421e-05,
  "timeoffset": 0,
  "connections": 3,
  "proxy": "",
  "difficulty": 0.0008245888870457042,
  "chain": "signet",
  "keypoolsize": 999,
  "paytxfee": 0.00000000,
  "balance": 10.00000000,
  "relayfee": 0.00001000,
  "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
}

./bitcoin-cli -datadir=signet stop

</p> </details>

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.

Made a few signet transactions using the GUI. Works well.

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

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!

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!

@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.

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

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

{
  "version": 209900,
  "blocks": 2875,
  "headers": 8376,
  "verificationprogress": 7.824597019403681e-230,
  "timeoffset": 0,
  "connections": 1,
  "proxy": "",
  "difficulty": 0.0007813477653418144,
  "chain": "signet",
  "keypoolsize": 1000,
  "paytxfee": 0.00000000,
  "balance": 0.00000000,
  "relayfee": 0.00001000,
  "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
}

• Created a signet address and received 10 signet BTCs:

./bitcoin-cli -datadir=signet getnewaddress
sb1q3tsk7vvs3guyg6cacxd8celepqjqupes4ura2t

./bitcoin-cli -datadir=signet setlabel sb1q3tsk7vvs3guyg6cacxd8celepqjqupes4ura2t brakmic-signet-address

./bitcoin-cli -datadir=signet getaddressinfo sb1q3tsk7vvs3guyg6cacxd8celepqjqupes4ura2t
{
  "address": "sb1q3tsk7vvs3guyg6cacxd8celepqjqupes4ura2t",                     
  "scriptPubKey": "00148ae16f31908a38446b1dc19a7c67f908240e0730",
  "ismine": true,
  "solvable": true,
  "desc": "wpkh([39028b98/0'/0'/0']03164fe243eda4c563657cc10b0c6f420fc92dde092f67b5e03f7d7ddfe20c9a6f)#l9u634et",
  "iswatchonly": false,
  "isscript": false,
  "iswitness": true,
  "witness_version": 0,
  "witness_program": "8ae16f31908a38446b1dc19a7c67f908240e0730",
  "pubkey": "03164fe243eda4c563657cc10b0c6f420fc92dde092f67b5e03f7d7ddfe20c9a6f",
  "ischange": false,
  "timestamp": 1588269529,
  "hdkeypath": "m/0'/0'/0'",
  "hdseedid": "0ba40a67510bf4857fd667d106e3f67c99257524",
  "hdmasterfingerprint": "39028b98",
  "labels": [
    "brakmic-signet-address"
  ]
}

./bitcoin-cli -datadir=signet gettransaction 2588fc33f81a53994648773591d4a6e127535c74b6340dee19e5608d9547a9ce
{
  "amount": 10.00000000,
  "confirmations": 0,
  "trusted": false,
  "txid": "2588fc33f81a53994648773591d4a6e127535c74b6340dee19e5608d9547a9ce",
  "walletconflicts": [
  ],
  "time": 1588269697,
  "timereceived": 1588269697,
  "bip125-replaceable": "no",
  "details": [
    {
      "address": "sb1q3tsk7vvs3guyg6cacxd8celepqjqupes4ura2t",
      "category": "receive",
      "amount": 10.00000000,
      "label": "brakmic-signet-address",
      "vout": 0
    }
  ],
  "hex": "0200000000010159fedd1084d29c3c4615254d1b3b5786c4f01c1a497e39762f40be3ec05b7b8d0000000000feffffff0200ca9a3b000000001600148ae16f31908a38446b1dc19a7c67f908240e073076c5f50500000000160014b342968beebfe1b0d17fe385e167567ad07811e902473044022053cc1d2a66739fb4b2ad6f284dff56cae186d7c9d464f4a1448ffcda4df95828022030f49b1c7d32544ce247d0a828254aa81b6ed315a00f37d225cee4585eeeb1f5012103c7f54159ca13bb20f515813df57de41cedae3c71dafc4b4150ab5b06ed72a527b8200000"
}

• My wallet balances (not confirmed)

./bitcoin-cli -datadir=signet getbalances                   
{
  "mine": {
    "trusted": 0.00000000,
    "untrusted_pending": 10.00000000,
    "immature": 0.00000000
  }
}

• Same balances after a few minutes

./bitcoin-cli -datadir=signet getbalances
{
  "mine": {
    "trusted": 10.00000000,
    "untrusted_pending": 0.00000000,
    "immature": 0.00000000
  }
}

@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 :)

@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 :)

uname -a
Linux 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.

./bitcoin-cli -datadir=signet uptime
39362

./bitcoin-cli -datadir=signet getblockchaininfo
{
  "chain": "signet",
  "blocks": 8464,
  "headers": 8464,
  "bestblockhash": "000002620188a0abe05b324d32ec943b71a593a6e2a577df6490af59079801c9",
  "difficulty": 0.0008142809685956302,
  "mediantime": 1588319843,
  "verificationprogress": 7.753189433667663e-09,
  "initialblockdownload": false,
  "chainwork": "0000000000000000000000000000000000000000000000000000000564eb9c63",
  "size_on_disk": 3244862,
  "pruned": false,

Awesome work, @kallewoof !

ACK 483fbce2b87de171112a0f304095797a6b5d3d08

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!

Removed inline; also replaced two Returns -1 if ... with Returns NO_WITNESS_COMMITMENT if ....

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

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

Error: SigNetParams: -signet_blockscript cannot be multiple values.

• Passed in a bad -signet_blockscript

[0%]...ERROR: CheckBlockSolution: Errors in block (block solution invalid)
2020-05-05T16:54:56Z ERROR: VerifyDB(): *** ReadBlockFromDisk failed at 9078, hash=0000045cba9402f733c6aeb5ab9dd6f68d898539b5bb0416c7260d09b80901de
2020-05-05T16:54:56Z : Corrupted block database detected.
Please restart with -reindex or -reindex-chainstate to recover.
: Corrupted block database detected.
Please restart with -reindex or -reindex-chainstate to recover.

A few comments follow if you re-touch.

Addressed @jonatack nits.

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.

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

Removed -signet_hrp (see #18267 (review))

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.

@brakmic Wow, very nice! Also, thanks for the heads up on 502 error. Investigating that.

@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.)

$ bash ./runsignet.sh
tr: Illegal byte sequence
tr: Illegal byte sequence
Creating backup datadir /Users/me/Downloads/signet/data/signet_datadir_backup
mkdir: /Users/me/Downloads/signet/data: No such file or directory
mkdir: /Users/me/Downloads/signet/data/signet_datadir_backup: No such file or directory
mkdir: /Users/me/Downloads/signet/data/signet_datadir_backup/signet: No such file or directory
Creating default bitcoin.conf
./runsignet.sh: line 44: /Users/me/Downloads/signet/data/signet_datadir_backup/bitcoin.conf: No such file or directory
-----------------------------------
  Getting Bitcoin Signet sources
-----------------------------------
Cloning into '/Users/me/Downloads/signet/data/signet'...
remote: Enumerating objects: 9, done.
remote: Counting objects: 100% (9/9), done.
remote: Compressing objects: 100% (7/7), done.
^Cceiving objects:  11% (19511/167291), 21.00 MiB | 4.31 MiB/s
$ ls
data          runsignet.sh  runsignet.sh~
$ ls data

@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.

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

$ bash ./runsignet.sh
tr: Illegal byte sequence
tr: 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. ;)

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

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

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

Error: 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!

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

error code: -28
error message:
Loading wallet...

now).

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

error code: -28
error message:
Loading 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. :)

@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.)

@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. :)

@kallewoof

Begging for coins from your faucet works now :)

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.

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?

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

<details><summary><code>git diff d4e204b 647c10f</code></summary><p>

$ git diff d4e204b 647c10f
diff --git a/src/chainparams.cpp b/src/chainparams.cpp
index 797b22fbe9..669010c53e 100644
--- a/src/chainparams.cpp
+++ b/src/chainparams.cpp
@@ -325,7 +325,7 @@ public:
         base58Prefixes[EXT_PUBLIC_KEY] = {0x04, 0x35, 0x87, 0xCF};
         base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94};
 
-        bech32_hrp = "sb" + args.GetArg("-signet_hrp", "");
+        bech32_hrp = "sb";
 
         fDefaultConsistencyChecks = false;
         fRequireStandard = true;
diff --git a/src/chainparamsbase.cpp b/src/chainparamsbase.cpp
index 4d5485e38a..78a87371bb 100644
--- a/src/chainparamsbase.cpp
+++ b/src/chainparamsbase.cpp
@@ -26,7 +26,6 @@ void SetupChainParamsBaseOptions()
     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);
     gArgs.AddArg("-signet", "Use the signet chain. Note that the network is defined by the signet_blockscript parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
     gArgs.AddArg("-signet_blockscript", "Blocks must satisfy the given script to be considered valid (only for signet networks)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
-    gArgs.AddArg("-signet_hrp", "Human readable part of bech32 address (suffixed by \"sb\"; default = \"\")", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
     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);
 }

</p></details>

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).

@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.

Merged all except WIP commit into current branch.

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.

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.

re-ACK 03ac167829d555de456d684f6580c6429253c7f0

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

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.

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.

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

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 .

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.

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 :-)

@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.

@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!

@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

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:

wallet_basic.py failed, Duration: 62 s
wallet_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

@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. :)

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.

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).

Addressed nits in to-squashies.

• 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)

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

• Faucet https://signet.bc-2.jp/
• Explorer https://explorer.bc-2.jp/

CR-Ack -- looked over, everything looks fine. Doesn't seem to interfere with any non-signet code paths.

ACK 4b96892b06a5f45e9dc1117dda0552822bfb5ce4

That's better than before. ACK 96e3d1e . I wish we can get signet soon, it will help to test taproot for example.

@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.

@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!)

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.

@jonatack Sorry about that. >< The changes are improvements, FWIW!

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.

@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.

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.

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:

diff --git a/src/Makefile.test.include b/src/Makefile.test.include
index 0068c9407..8eb417cbe 100644
--- a/src/Makefile.test.include
+++ b/src/Makefile.test.include
@@ -134,6 +134,7 @@ FUZZ_TARGETS = \
   test/fuzz/scriptnum_ops \
   test/fuzz/service_deserialize \
   test/fuzz/signature_checker \
+  test/fuzz/signet \
   test/fuzz/snapshotmetadata_deserialize \
   test/fuzz/span \
   test/fuzz/spanparsing \
@@ -1106,6 +1107,12 @@ test_fuzz_signature_checker_LDADD = $(FUZZ_SUITE_LD_COMMON)
 test_fuzz_signature_checker_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
 test_fuzz_signature_checker_SOURCES = test/fuzz/signature_checker.cpp

+test_fuzz_signet_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES)
+test_fuzz_signet_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
+test_fuzz_signet_LDADD = $(FUZZ_SUITE_LD_COMMON)
+test_fuzz_signet_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
+test_fuzz_signet_SOURCES = test/fuzz/signet.cpp
+
 test_fuzz_snapshotmetadata_deserialize_CPPFLAGS = $(AM_CPPFLAGS) $(BITCOIN_INCLUDES) -DSNAPSHOTMETADATA_DESERIALIZE=1
 test_fuzz_snapshotmetadata_deserialize_CXXFLAGS = $(AM_CXXFLAGS) $(PIE_FLAGS)
 test_fuzz_snapshotmetadata_deserialize_LDADD = $(FUZZ_SUITE_LD_COMMON)
diff --git a/src/test/fuzz/signet.cpp b/src/test/fuzz/signet.cpp
new file mode 100644
index 000000000..5c2979abe
--- /dev/null
+++ b/src/test/fuzz/signet.cpp
@@ -0,0 +1,35 @@
+// Copyright (c) 2020 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <chainparams.h>
+#include <consensus/validation.h>
+#include <primitives/block.h>
+#include <signet.h>
+#include <streams.h>
+#include <test/fuzz/fuzz.h>
+#include <test/fuzz/FuzzedDataProvider.h>
+#include <test/fuzz/util.h>
+
+#include <cstdint>
+#include <optional>
+#include <vector>
+
+void initialize()
+{
+    InitializeFuzzingContext(CBaseChainParams::SIGNET);
+}
+
+void test_one_input(const std::vector<uint8_t>& buffer)
+{
+    FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
+    const std::optional<CBlock> block = ConsumeDeserializable<CBlock>(fuzzed_data_provider);
+    if (!block) {
+        return;
+    }
+    (void)CheckBlockSolution(*block, Params().GetConsensus());
+    if (GetWitnessCommitmentIndex(*block) != NO_WITNESS_COMMITMENT) {
+        (void)SignetTxs(*block, ConsumeScript(fuzzed_data_provider));
+    }
+}

@jonatack I won't rebase/squash anything until you give me the go ahead.

@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. <strike>Functional tests to sanity check the conf options handling would be good as well; I'll propose a commit later today.</strike>

Edit: thanks for the updates, LGTM.

Unit tests util_tests still failing starting from 18e18820bb.

@jonatack Sorry, forgot about that comment. Checking now.

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

@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.

Yeah, sorry, wallet_groups CI failure.

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.

$ test/fuzz/signet
INFO: Seed: 1520911288
INFO: Loaded 1 modules   (328861 inline 8-bit counters): 328861 [0x5567902be7d8, 0x55679030ec75), 
INFO: Loaded 1 PC tables (328861 PCs): 328861 [0x55679030ec78,0x556790813648), 
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
[#2](/bitcoin-bitcoin/2/)	INITED cov: 363 ft: 364 corp: 1/1b exec/s: 0 rss: 121Mb
.../...
[#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-
[#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-
[#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-

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)

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:

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

ACK 15a26087a3405903c562fce8c8d7682731b807c8

<details><summary>Show Signature</summary>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 15a26087a3405903c562fce8c8d7682731b807c8
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAl88XHkACgkQ5+KYS2KJ
yTrgaw/+Pb5Fpa7D8unEoaQ5koxvzhY9PMIvwbe+SA3arAsDVSf/8duHM1o7+ZVD
mZgB02tm4JUGWD58MGnCsrKRkXa8MHikilPzAfFUb6IKeFCfJoK9/FUN0zq/w0ve
phH26Zvw2KzPfkrZcyDXKOwkvOYXN/ro2a0IhjATMXQRJ26fltCVBiDcahxC4I8g
bsDwsCfDx4dDCR6HT/dvrrph+EVZ+nPXYlqdhhXGdYiQvifgvAUr9fTg3dUUL2W5
pfZv167yGz3G+5Bgy1kJjTNWLsOKnie881pJIas/Gl//SMm8VBcrYLRxoGICNj6f
Kln9rufGyeNJI4RYsRFohmRmIlso9f/LHMTTb3A7qn1l9+gBUaBjOlshNs24kQLt
6NsOxJsfnkyeeonDcvGHBm2LF/pzIPBRbzN3w5c31p6deYoVtCve/kW66zOs4xa+
/u4Ig6MOUCTqv13UP/Oiu85Rhim9hxaI+32LABm6C7MXcIeIoRr1uGHsePr7tnKg
C3MzyGzKYqsYXKkDGCu7M7P7Ab40wkBTyQLBSnANtPnu/EK7F2qqWs1sR4VDCiq1
NHq2m57LtgYc+iro6WLQS2QQm2SSyXwlXcvWuDuMGDDA+k9/MrqdnQvO7FfHwLoC
veCwBsy04NPxt8X4C+SY7GSnqDDWKT9efVUh43IkBwaUwGxTrio=
=7d3n
-----END PGP SIGNATURE-----

pinheadmz's public key is on keybase

</details>

@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.)

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:

2020-08-19T12:58:27Z ERROR: ConnectBlock(): coinbase pays too much (actual=5000113810 vs limit=5000081890)
2020-08-19T12:58:27Z InvalidChainFound: invalid block=00000899df7024bf4d015a59a89333a3e167e4c9c3667c42a72843a52f3cf751  height=1296  log2_work=28.919402  date=2020-08-19T12:58:25Z
2020-08-19T12:58:27Z InvalidChainFound:  current best=000021da20b30538459493b10ad54460114d05e4187751c73f26b24889e28201  height=1295  log2_work=28.918289  date=2020-08-19T12:48:21Z
2020-08-19T12:58:27Z ERROR: ConnectTip: ConnectBlock 00000899df7024bf4d015a59a89333a3e167e4c9c3667c42a72843a52f3cf751 failed, bad-cb-amount
2020-08-19T12:58:27Z InvalidChainFound: invalid block=00000899df7024bf4d015a59a89333a3e167e4c9c3667c42a72843a52f3cf751  height=1296  log2_work=28.919402  date=2020-08-19T12:58:25Z
2020-08-19T12:58:27Z InvalidChainFound:  current best=000021da20b30538459493b10ad54460114d05e4187751c73f26b24889e28201  height=1295  log2_work=28.918289  date=2020-08-19T12:48:21Z
noticed t2020-08-19T13:24:18Z Potential stale tip detected, will try using extra outbound peer (last tip update: 2153 seconds ago)
2020-08-19T13:34:48Z Potential stale tip detected, will try using extra outbound peer (last tip update: 2783 seconds ago)

@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!

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.