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)
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
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
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
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..
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
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) {
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!
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));
-1
commitment index values in block.h/.cpp
with a static constant whose name could also make their meaning explicit
-1
is used.
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");
"([^"]*)testnet
and all instances I found used lowercase except for start-of-sentence-uppercasing.
SigNet
as well in comments and log messages.
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]);
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]);
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");
powLimit
value?
323+
324+ bech32_hrp = "sb" + args.GetArg("-signet_hrp", "");
325+
326+ fDefaultConsistencyChecks = false;
327+ fRequireStandard = true;
328+ m_is_test_chain = true;
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;
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);
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);
CConnman
options in init.cpp
, but I can’t see that using the chain parameter stuff, so… yeah, not sure.
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);
Human readable protocols greatly reduce the cost of debugging.[1]
.
signet_hrp
being a run-time option?
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.
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.
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
s/suffixed/prefixed/?
Maybe add a usage example.
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.
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);
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?
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) {
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)) {
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++);
63b5d7a3a7e6081e3b42a78ad1f nit: would it be better to construct rather than copy?
0- result.push_back(*pc++);
1+ result.emplace_back(*pc++);
emplace_back
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");
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.
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.
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.
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). */
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). */
ISTM the first version is correct?
That’s why it’s a nit.
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?
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.
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__));
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);
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");
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.
-seednode
is passed and echo it rather than only “Using default signet network”
args
not gArgs
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):
src/test/util_tests.cpp:870/871
and src/test/key_io_tests.cpp:139
might make sense in this pull alreadyFirst 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
andsrc/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!
251@@ -249,13 +252,95 @@ class CTestNetParams : public CChainParams {
252 }
253 };
254
255+/**
256+ * SigNet
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 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
andsrc/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:
./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}
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}
0./bitcoin-cli -datadir=signet getbalances
1{
2 "mine": {
3 "trusted": 0.00000000,
4 "untrusted_pending": 10.00000000,
5 "immature": 0.00000000
6 }
7}
0./bitcoin-cli -datadir=signet getbalances
1{
2 "mine": {
3 "trusted": 10.00000000,
4 "untrusted_pending": 0.00000000,
5 "immature": 0.00000000
6 }
7}
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 :)
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
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!
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) {
I still don’t understand why this is moved #16411 (review)
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.template<>
s. No need to specify it twice.
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.
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.
signet.h/cpp
, and is now included in the add signet basic support
commit333@@ -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 */
-1
. Will fix at next rebase.
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++) {
++s
is preferred over s++
per developer-notes.md::L94
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 &&
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)
?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.
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");
emplace_back
like the other vSeeds.emplace_back operations in this file.
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+ }
-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.
Light re-ACK 3e44470efaafa.
Code review, build/tests, ran bitcoind and the GUI and focused a bit more on the -signet_blockscript
config option:
0Error: SigNetParams: -signet_blockscript cannot be multiple values.
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.
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]);
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.
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.
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);
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.
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:
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.
~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 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
@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 :)
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. ;)
@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!
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).
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. :)
@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. :)
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:
GetWitnessCommitmentIndex
GetWitnessCommitmentSection
to signet.cpp-signet_hrp
g_signet_blockscript
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
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 }
@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.
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.
9+#include <config/bitcoin-config.h>
10+#endif
11+
12+#include <consensus/params.h>
13+
14+#include <stdint.h>
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
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>{});
in commit 7168395ea63d33075a60369f5778eb635208beba
I think the ignored return value should be documented
0 (void)SetWitnessCommitmentSection(mtx, SIGNET_HEADER, std::vector<uint8_t>{});
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
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:
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.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.
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);
src/validation.h
).
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
scriptCode
unused ever, maybe drop a comment saying it’s just there due to inheritance?
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.
scriptCode
) in declaration/definition.
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)) {
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.
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.
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;
state.Invalid
line for better debugging?
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;
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.
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.
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);
chain.NetworkIDString()
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]);
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");
OP_1 03ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be430 OP_1 OP_CHECKMULTISIG
Why did you choose 1-of-1 multisig instead of a single OP_CHECKSIG
?
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;
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?
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.
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);
const std::array<uint8_t, 4>
(instead of const uint8_t header[4]
) and giving SIGNET_HEADER
the same treatment?
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 );
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.
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:
src/test/data/key_io_valid.json
?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?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:
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:
— 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.
326+ base58Prefixes[EXT_SECRET_KEY] = {0x04, 0x35, 0x83, 0x94};
327+
328+ bech32_hrp = "sb";
329+
330+ fDefaultConsistencyChecks = false;
331+ fRequireStandard = true;
false
for testnet. Is the idea just to be more mainnet-like?
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]
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 :-)
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);
consensus/validation.h
.
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.
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.
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
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. :)
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
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_} { }
359badf294c8fa632b7df479a3e146cf28658f6b
nit, args can have the same name as members.
m_*
is worthwhile, though, so will do that.
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};
359badf294c8fa632b7df479a3e146cf28658f6b
nit, just noting signet_blocks
is only used in the next commit.
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);
359badf294c8fa632b7df479a3e146cf28658f6b
Why have this separated? Could be in the to the constructor?
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;
359badf294c8fa632b7df479a3e146cf28658f6b
These could be const?
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+ {
359badf294c8fa632b7df479a3e146cf28658f6b
Can’t find a reason for this block. Is this a leftover?
consensus/validation.h
stuff, removing the transaction based GetWitnessCommitmentIndex
variantTest network was restarted (Signet Global Test Net V) along with below services:
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)) {
block.GetHash() != consensusParams.hashGenesisBlock
check into CheckBlockSolution
?
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};
@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.
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.
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.
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)");
0 return error("CheckBlockSolution: Errors in block (no witness commitment)");
171+ vout.scriptPubKey[5] == 0xed) {
172+ commitpos = o;
173+ }
174+ }
175+ }
176+ return commitpos;
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;
126+ // genesis block solution is always valid
127+ return true;
128+ }
129+
130+ int cidx = GetWitnessCommitmentIndex(block);
131+ if (cidx == NO_WITNESS_COMMITMENT) {
nit: can be shortened (or make cidx
const).
0 if (GetWitnessCommitmentIndex(block) == NO_WITNESS_COMMITMENT) {
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);
0 const int cidx = GetWitnessCommitmentIndex(block);
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.
10+#include <primitives/block.h>
11+#include <uint256.h>
12+
13+#include <cstdint>
14+#include <vector>
15+#include <array>
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>
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.");
6920af6 nit, consistent order
0 throw std::runtime_error("Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.");
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));
6920af6 nit: while here, it would be nice to add brackets to these successive conditionals
edit: idem in CreateChainParams
, line 485
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);
test/util_tests.cpp
at 6920af65. Feel free to ignore the nits below.
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));
block.vtx.empty()
isn’t handled here. What about handling it by returning invalid, or alternatively asserting that !block.vtx.empty()
?
s/cb/modified_cb/
?
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+}
@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.
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");
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.
util_tests
still failing starting from 18e18820bb.
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-
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
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)
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.
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;
“script” is really not self-documenting.
witness_commitment
seems a better name
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)
s/ExtractCommitmentSection/ExtractCommitmentSectionAndModifyCB/
?
s/script/witness_commitment_script_to_replace/
or something? really would like this more self-documenting
FetchAndClearCommitmentSection
.
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;
s/found/found_header/
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();
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)) {
s/data/witness_data/
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)
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();
to_spend_data
Just make a new vector named to_spend_data
or block_data
(as it is called in the BIP)
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)
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?
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.
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.
static
.
12+
13+#include <array>
14+#include <cstdint>
15+#include <vector>
16+
17+#include <span.h>
in commit 0855e12485848b578bd2796d24d5a16cbb763b72:
Are any of the includes here used in the header? (Except for the first three)
signet.cpp
.
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)");
Create
is replaced with a return
)
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()
.
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.
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.
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 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.
Concept ACK, feel free to ignore the other style-related feedback
1cc60a05426b02f86f63a2d3f3744ae484e1ac71
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)");
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}) {
16@@ -17,35 +17,43 @@
17 #include <util/strencodings.h>
18 #include <util/system.h>
19
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+ }
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
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)
CheckSignetBlockSolution
to be extra verbose. Also, could add a if (!consensusparams.signet_blocks) return false;
or assert(false);
Approach ACK 2e539c8fce 🔵
Changes since last review:
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 -
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?
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?
@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?
git diff 15a2608 2e539c8
, re-reviewed the code in the changed areas, tested running and using signet via the cli.
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.
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?
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.
The test: basic signet tests
commit now has a bunch of tests.
Also changed:
chainparams.cpp
:
gArgs
→ args
powLimit
has been modified to exactly match the compact variant (in genesis) (this is not a hard fork)signet.cpp
:
OP_TRUE
challenge is now accepted as a valid challenge/solution (used in the first part of the new feature_signet.py
tests)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":
Invalid private key encoding (-5)
)
Probably another reason why it would be better to have the same prefix. See also:
rust-bitcoin
relies on bech32 prefixes to be unique per network. This change breaks that assumption.
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.
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":
cache_node
is perhaps unable to generate any blocks at all, due to lacking the privkeys to properly sign them.
setup_clean_chain==False
100+
101+ height = 0
102+ try:
103+ node.submitblock(signet_blocks[0])
104+ assert False # this should fail
105+ except: