BIP-325: Signet support

kallewoof commented at 5:11 am on July 18, 2019: member

This PR implements BIP 325 (https://github.com/bitcoin/bips/blob/master/bip-0325.mediawiki) – Signet support.

Note: This PR is being split into more bite-sized PR’s, the first of which is #18267.

Merged check boxes:

Signet consensus: #18267
Signet RPC tools: pending consensus merge
Signet utility scripts (contrib/signet): pending RPC tool merge

Signet is a proposed new type of test network that requires a signature in the blocks. While this kind of network is centralized, it allows for properties that are desirable in a test network, which testnet alone cannot provide.

Signet has a default global testnet baked in for easy usage, but is built around the concept of there being multiple simultaneous signets. Someone is already working on a signet with bip-taproot etc patched on top of it. When new features are tested globally, starting a temporary signet for that purpose is trivial.

Noteworthy:

~~proof of work check now skips genesis block for all networks, including mainnet~~
a new global “is signet” flag is added, which affects consensus: it adds a new enforcement that all blocks must have a valid signature for the “signet block script”, a global script
DecodeHexBlk will now attempt to load the block data from a file whose filename is the hex string argument, if it turns out to not be a valid hex string; this has the odd caveat of the relative directory being relative to the execution of bitcoind, not bitcoin-cli (see contrib/signet/mkblock.sh)

Update: proof of work check is back in place; instead, Signet now requires an additional -signet_genesisnonce parameter to operate.

fanquake added the label Tests on Jul 18, 2019

fanquake added the label Needs Conceptual Review on Jul 18, 2019

junderw commented at 5:49 am on July 18, 2019: contributor

Brainstorming some things we should consider:

01. Does adding this put anyone at risk?
12. Does adding this somehow hurt Bitcoin's code base?
23. Does adding this require some long term maintenance / increase the weight on devs for support in the future?

Since Testnet has been used at some points to scam people out of money, I could see how this could be used in a similar way. However, with something like this it would be easier for someone to create a signet and give it value (aka use it to create a permission-ed altcoin and mooch 100% off Bitcoin Core development. At least with current altcoin, they have a “keep up to date with merging Core patches” barrier of entry… this would not have such a barrier.)
I would say no. But that would need more in-depth technical review.
If scams run rampant in my scenario in number 1, some people might propose hard coding blacklists that will require scammers to maintain patches removing themselves from the signet blacklist etc… But other than that, I don’t see much problem.

I am leaning towards concept ACK. But I am slightly concerned about someone abusing this like testnet has been abused in the past, and how signet’s differences make those abuses easier or harder.

kallewoof commented at 5:57 am on July 18, 2019: member

@junderw

Thanks for feedback, comments below:

1. Does adding this put anyone at risk?

Example?

2. Does adding this somehow hurt Bitcoin's code base?

There is a tiny amount of risk involved as it touches consensus validation code, but this is a tiny code change.

If for some reason someone managed to flip the “signet” flag on a running node, for example, that would cause those nodes to start rejecting bitcoin blocks. I don’t foresee this as very likely, personally.

3. Does adding this require some long term maintenance / increase the weight on devs for support in the future?

Yes, same as adding any feature to a system. I assume the point of your question is if the added maintenance is worth it; in my opinion it is. Signet is fairly self-contained (3 main parts: signet, wallet RPC, miner RPC, and some sprinkled stuff; there are also the contrib/signet scripts but I see those as irrelevant in this case as they’re not a part of the C++ codebase), so it shouldn’t be that hard to maintain.

4. Since Testnet has been used at some points to scam people out of money, I could see how this could be used in a similar way. However, with something like this it would be easier for someone to create a signet and give it value (aka use it to create a permission-ed altcoin and mooch 100% off Bitcoin Core development. At least with current altcoin, they have a "keep up to date with merging Core patches" barrier of entry... this would not have such a barrier.)

I don’t think this is an issue, to be honest. People can already copy the codebase and tweak it to make a new coin, or start up a regtest and ask their victim to connect to it using This Special Bitcoin.conf File [tm] which so happens to have regtest=1 in it.

junderw commented at 6:22 am on July 18, 2019: contributor

People can already copy the codebase and tweak it to make a new coin, or start up a regtest and ask their victim to connect to it using This Special Bitcoin.conf File [tm] which so happens to have regtest=1 in it.

Which is why I am mostly leaning toward concept ACK.

But tbh, if someone said “hey, we have a 7-of-13 multisig based faster Bitcoin!” as compared to “hey use this regtest thing, totally decentralized, we swear!” could be argued by pedantic people as “not technically false”… so a regtest based scam has less of a chance of someone understanding the tech aspect saying “yeah, that’s not centralized” than a signet based scam.

Also, there are also many scams that copy-pasted the code base, sure… but with a signet based scam, they don’t even need to maintain a patch. Just tell the victims to download Bitcoin Core. And it works out of the box (with a conf tweak)…

So while the risk is super low, it is objectively higher than regtest and testnet…

That being said. The benefit is great compared to that risk. Which, again, is why I say “leaning towards concept ACK.”

It would be very powerful to have someone who already has Bitcoin Core downloaded and running to spin up another instance with some args that lets them try out taproot etc etc…

kallewoof commented at 6:35 am on July 18, 2019: member

Yep, I see what you’re saying. Ultimately both cases require a conf tweak, though (regtest or testnet scam vs signet scam), and signet case requires them to give you a big ugly signet challenge and signet seed node(s), so I’m not sure how viable the attack really is in the end.

NicolasDorier commented at 8:01 am on July 18, 2019: contributor

Concept ACK, though I am unsure if signet should be configurable at all. (via signet_blockscript for example)

It should just be a shared public network, an alternative (or replacement) of testnet.

DrahtBot commented at 8:03 am on July 18, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18267 (BIP-325: Signet [consensus] by kallewoof)
#18002 (Abstract out script execution out of VerifyWitnessProgram() by sipa)
#17977 ([WIP] Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa)
#17693 (rpc: Add generateblock to mine a custom set of transactions by andrewtoth)
#17037 (Testschains: Many regtests with different genesis and default datadir by jtimon)
#17032 (Tests: Chainparams: Make regtest almost fully customizable by jtimon)
#16653 (script: add simple signature support (checker/creator) by kallewoof)
#15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
#15481 (Restrict timestamp when mining a diff-adjustment block to prev-600 by TheBlueMatt)
#13062 (Make script interpreter independent from storage type CScript by sipa)

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.

NicolasDorier commented at 8:04 am on July 18, 2019: contributor

The code does not seems to be that much to maintain, I would advise though to open a separate pull requests for most changes in validation.cpp/h.

laanwj added this to the "Chasing Concept ACK" column in a project

kallewoof force-pushed on Jul 19, 2019

in src/validation.cpp:969 in 88312cd89e outdated

940@@ -941,9 +941,12 @@ bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::P
941         return error("%s: Deserialize or I/O error - %s at %s", __func__, e.what(), pos.ToString());
942     }
943 
944-    // Check the header
945-    if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
946-        return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
947+    // We skip POW checks for genesis block
948+    if (block.GetHash() != consensusParams.hashGenesisBlock) {

NicolasDorier commented at 7:42 am on July 19, 2019:

I would move this test inside CheckProofOfWork

kallewoof commented at 8:26 am on July 19, 2019:

I need this if block for CheckBlockSolution too, though.

https://github.com/bitcoin/bitcoin/blob/587526d21850e71bf7c7685a6c40e4084b4d894a/src/validation.cpp#L945-L952

in src/validation.cpp:3041 in 88312cd89e outdated

2967@@ -2965,10 +2968,13 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P
2968     if (block.fChecked)
2969         return true;
2970 
2971-    // Check that the header is valid (particularly PoW).  This is mostly
2972-    // redundant with the call in AcceptBlockHeader.
2973-    if (!CheckBlockHeader(block, state, consensusParams, fCheckPOW))
2974-        return false;
2975+    // We skip POW checks for genesis block
2976+    if (block.GetHash() != consensusParams.hashGenesisBlock) {

NicolasDorier commented at 7:43 am on July 19, 2019:

I would move this test inside CheckBlockHeader

kallewoof commented at 8:27 am on July 19, 2019:

I need this if block for CheckBlockSolution too, though.

https://github.com/bitcoin/bitcoin/blob/587526d21850e71bf7c7685a6c40e4084b4d894a/src/validation.cpp#L2974-L2982

NicolasDorier commented at 11:45 am on July 19, 2019:

CheckBlockSolution could also return true if block.GetHash() == consensusParams.hashGenesisBlock

kallewoof commented at 12:32 pm on July 19, 2019:

That’s fair. But the check appears in 3 places instead of 2. Not sure why that is better, tbh.

Edit: to clarify, moving the check to the three called-to functions means the check will happen 3 times in 3 different places, compared to 2 times in 2 places, as is the case right now.

MarcoFalke commented at 3:40 pm on July 19, 2019: member

Would be nice to have a contrib/signet/README.md to explain what those sh scripts are supposed to be doing

kallewoof force-pushed on Jul 20, 2019

kallewoof commented at 3:25 am on July 20, 2019: member

@MarcoFalke Added README file. Also missing release notes. Since it is Needs Concept Review stage, I am not gonna rush that just yet.

kallewoof commented at 3:32 am on July 20, 2019: member

I added a “noteworthy” section to the OP of this PR, which points out a number of changes to existing functionality.

practicalswift commented at 11:41 am on July 20, 2019: contributor

Concept ACK

Thanks for doing this - would be very useful for testing.

kallewoof force-pushed on Jul 30, 2019

in contrib/signet/README.md:44 in ac9da7d232 outdated

39+mkblock.sh
40+==========
41+
42+A script to generate one Signet block.
43+
44+Syntax: `mkblock.sh <bitcoin-cli path> [<bitcoin-cli args>]`

MarcoFalke commented at 5:23 pm on July 30, 2019:

How is this supposed to work? It looks like the chainparams are setting the pow to the pow of mainnet. This script calls grindblock, which is a non-optimized while loop on a single CPU core. Could this lead to the chain stalling?

junderw commented at 11:33 pm on July 30, 2019:

If the signers compete with each other, or frequently change their hashpower, yes.

I think the idea is that signatures limit the miner pool to a few people, then those people each pledge a consistent low hashpower to the network.

Prevents people from hopping on with ASICs, driving up difficulty and then disappearing.

kallewoof commented at 0:17 am on July 31, 2019:

The pow is not really the pow of mainnet unless you don’t pass -signet or don’t use a -datadir with signet in bitcoin.conf.

grindblock is basically equivalent to generate*. You can use other miners like existing CPU miners or even ASICs (though you can’t use the extra nonce so you’d have to resign the block instead).

DrahtBot added the label Needs rebase on Aug 2, 2019

kallewoof force-pushed on Aug 3, 2019

DrahtBot removed the label Needs rebase on Aug 3, 2019

kallewoof force-pushed on Aug 5, 2019

MarcoFalke referenced this in commit d5ea8f4bf3 on Aug 5, 2019

DrahtBot added the label Needs rebase on Aug 5, 2019

kallewoof force-pushed on Aug 5, 2019

kallewoof commented at 3:03 pm on August 5, 2019: member

Refactored now that #16509 is in (one less commit, and fewer (test) code lines touched!).

kallewoof force-pushed on Aug 5, 2019

DrahtBot removed the label Needs rebase on Aug 5, 2019

kallewoof force-pushed on Aug 5, 2019

michaelfolkson commented at 2:01 pm on August 8, 2019: contributor

Is there a resource online discussing the difference(s) between regtest and (proposed) signet? I found this on the bitcoin-dev mailing list but it doesn’t go into much detail. Is unvetted “people rewriting the blockchain at their whim by mining a ton of blocks” a common problem? It wouldn’t be a problem for a training workshop (just spin up a new one) but could possibly be for a project building on top of regtest.

Also any other key differences between design of regtest and signet? I don’t need convincing of signet’s upsides in comparison to testnet but not convinced re regtest versus signet. Digging into understanding regtest code a bit more but any guidance would be appreciated.

I’ve opened up a Stack Exchange page for this question so probably better to move answers there.

kallewoof commented at 1:36 am on August 9, 2019: member

@michaelfolkson I already replied on Twitter, but there is a Bitcoin Wiki entry and also a Bitcoin Stack Exchange page (made by you).

sidhujag referenced this in commit 511873dd22 on Aug 10, 2019

DrahtBot added the label Needs rebase on Aug 15, 2019

kallewoof force-pushed on Aug 16, 2019

DrahtBot removed the label Needs rebase on Aug 16, 2019

kallewoof force-pushed on Aug 16, 2019

kallewoof force-pushed on Aug 19, 2019

kallewoof commented at 4:24 am on August 19, 2019: member

The proof of work changes were removed; Signet now requires a POW-valid genesis block, which is generated using grindblock <challenge> and which is provided to nodes using -signet_genesisnonce.

kallewoof force-pushed on Aug 19, 2019

NicolasDorier commented at 4:44 am on August 19, 2019: contributor

Update: proof of work check is back in place; instead, Signet now requires an additional -signet_genesisnonce

It should not require. If I want to use signet, I want to do bitcoind -signet without any more obscure parameter.

kallewoof commented at 4:50 am on August 19, 2019: member

@NicolasDorier The default parameters include a genesis nonce value. It’s only for custom signets, where you provide your own -signet_blockscript as well.

kallewoof force-pushed on Aug 19, 2019

in src/chainparams.cpp:11 in ac9147c9f7 outdated

 6@@ -7,6 +7,8 @@
 7 
 8 #include <chainparamsseeds.h>
 9 #include <consensus/merkle.h>
10+#include <hash.h> // for signet block challenge hash
11+#include <signet.h>

NicolasDorier commented at 7:20 am on August 19, 2019:

I think you are not using this one in this file.

kallewoof commented at 7:50 am on August 19, 2019:

I do; I reference g_signet_blocks, for example.

jtimon commented at 10:41 pm on September 10, 2019:

I think it’s only for that, right? Perhaps a small reason to put the global here instead of there.

kallewoof commented at 1:18 pm on March 5, 2020:

Belated, but I am removing this and adding an extern CScript g_signet_blockscript in the signet chainparams class.

in src/core_read.cpp:179 in ac9147c9f7 outdated

175 bool DecodeHexBlk(CBlock& block, const std::string& strHexBlk)
176 {
177-    if (!IsHex(strHexBlk))
178+    if (!IsHex(strHexBlk)) {
179+        std::string actual;
180+        if (ReadFromDisk(strHexBlk, actual)) return DecodeHexBlk(block, actual);

NicolasDorier commented at 7:25 am on August 19, 2019:

why do you need this?

kallewoof commented at 7:48 am on August 19, 2019:

Juggling block hex values in Bash is very fragile. If a block gets too big, bash scripts start breaking. To avoid, the issuer script redirects output to files during the signing/grinding process.

in src/rpc/mining.cpp:236 in ac9147c9f7 outdated

182@@ -171,6 +183,10 @@ static UniValue generatetoaddress(const JSONRPCRequest& request)
183         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Error: Invalid address");
184     }
185 
186+    if (g_signet_blocks) {
187+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Generating signet blocks require that you use getnewblockhex");

NicolasDorier commented at 7:27 am on August 19, 2019:

is there really no way to make it works, can’t you call getnewblockhex?

kallewoof commented at 7:49 am on August 19, 2019:

Recently, the wallet code and the rest of the code have been separated. Unfortunately this means the mining part and the signing part have to be separated as well.

NicolasDorier commented at 7:53 am on August 19, 2019:

I don’t understand. Why are you talking about wallet code? generatetoaddress does touch only mining code and getnewblockhex is in the scope of this function.

kallewoof commented at 5:04 am on September 11, 2019:

I think we discussed this outside of here, but basically, you don’t have the private keys associated with any addresses, so you’d have to provide those every time here. I don’t think we should encourage systems that behave in that manner (“bitcoin core does it so it must be safe”).

in src/script/sign.h:35 in ac9147c9f7 outdated

31@@ -32,6 +32,16 @@ class BaseSignatureCreator {
32     virtual bool CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& keyid, const CScript& scriptCode, SigVersion sigversion) const =0;
33 };
34 
35+class SimpleSignatureCreator : public BaseSignatureCreator

NicolasDorier commented at 7:29 am on August 19, 2019:

comments about the purpose of this would be nice

kallewoof commented at 7:54 am on August 19, 2019:

Added a few words to describe what it is. :)

kallewoof force-pushed on Aug 19, 2019

NicolasDorier commented at 8:24 am on August 19, 2019: contributor

So I discussed with @kallewoof I don’t like the fact that generatetoaddress does not work on custom signets. The reason why generatetoaddress does not work on signet is that signet need blocks to be signed, and right now it uses the bitcoin wallet to find the keys. However generatetoaddress can’t depends on wallet code.

Now we have three ways of fixing the problem:

Passing the keys for signing on signet via command line --signet_privatekey (My preference)
Passing the keys to generatetoaddress (@kallewoof preference)
We don’t care for this PR.

I don’t like 2, because it does not solve my initial problem: to have a custom signet just work with the same code you would use for regtest. @kallewoof does not like my preference, because he says passing private keys via command line is bad practice. Personally, I don’t think this is a problem because custom signet coins have no value, so no damage can be done.

Sjors commented at 8:37 am on August 19, 2019: member

@NicolasDorier or create generatetodescriptor which uses the block height as the index.

practicalswift commented at 8:57 am on August 19, 2019: contributor

@NicolasDorier @kallewoof Is the risk scenario that the signet private key would leak via process listings? If that it deemed a problem then it could perhaps be passed as an environment variable?

NicolasDorier commented at 9:19 am on August 19, 2019: contributor

@Sjors I don’t really understand what you mean, but anyway there should not have “special way of doing same thing” in RPC depending if you are on custom signet or regtest. @practicalswift it feel a bit weird to use environment variable when everything else is not. I don’t think that the key leaking is in anyway a problem that need to be solved.

Sjors commented at 9:32 am on August 19, 2019: member

@NicolasDorier instead of providing a single address or private key, you provide a descriptor so that each block will have a unique payout address. E.g. if you pass wpkh(xpub/0/*) it could replace * with the block height. You can use importmulti with the private key wpkh(xpriv/0/*) to import that descriptor in any wallet, from which you can then spend the coins.

practicalswift commented at 3:56 pm on August 19, 2019: contributor

it feel a bit weird to use environment variable when everything else is not.

Yes, that’s a good point.

Perhaps I’m missing something but wouldn’t setting signet_privatekey in the config file solve the key-leak-via-process-listing issue (assuming it is an issue)?

kallewoof commented at 6:12 am on August 20, 2019: member

I don’t agree with the premise that the RPC generatetoaddress must be possible under Signet. If you are generating the blocks yourself, use regtest, or use the Signet solution available. Signet is for more global testing where you often are not the miner.

I’m neutral on the private key juggling, as long as we don’t teach people bad habits for valuable private keys.

p.s. I think you guys are talking about different things. @NicolasDorier wants signet to seamlessly work exactly like regtest, including mining; @Sjors seems to be talking about the actual generated-to addresses and their reuse?

kallewoof force-pushed on Aug 20, 2019

NicolasDorier commented at 11:50 am on August 22, 2019: contributor

@Sjors I see, well my complaint is generally about, as a RPC user, I expect that RPC works as it would with regtest. If I need to make an if/else in my code to specifically handle the case of a custom signet this would really suck.

in src/validation.cpp:972 in 39aeff9902 outdated

968@@ -968,6 +969,8 @@ bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::P
969     if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
970         return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
971 
972+    if (g_signet_blocks && block.GetHash() != consensusParams.hashGenesisBlock && !CheckBlockSolution(block, consensusParams)) return false; /* function calls error(..) */

jtimon commented at 2:39 pm on August 22, 2019:

instead of g_signet_blocks we could use consensusParams.signet_blocks or similar.

kallewoof commented at 4:56 am on August 23, 2019:

Did this, but did not move blockscript - see detached comment.

in src/validation.cpp:3320 in 39aeff9902 outdated

3038@@ -3036,6 +3039,8 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P
3039     if (!CheckBlockHeader(block, state, consensusParams, fCheckPOW))
3040         return false;
3041 
3042+    if (g_signet_blocks && fCheckPOW && block.GetHash() != consensusParams.hashGenesisBlock && !CheckBlockSolution(block, consensusParams)) return false;
3043+

jtimon commented at 2:42 pm on August 22, 2019:

Perhaps do this in CheckBlockHeader? That way unsigned blocks would be detected faster. Also, I think the return false in a new line would be more readable.

kallewoof commented at 4:57 am on August 23, 2019:

CheckBlockSolution requires the block, not just the header.

Moving return to own line for readability.

in src/signet.cpp:15 in 3022803f33 outdated

11+#include <script/interpreter.h>
12+#include <script/standard.h>        // MANDATORY_SCRIPT_VERIFY_FLAGS
13+#include <util/system.h>
14+
15+bool g_signet_blocks = false;
16+CScript g_signet_blockscript;

jtimon commented at 2:45 pm on August 22, 2019:

Likewise this can be in consensus params instead of being a new global.

kallewoof commented at 4:59 am on August 23, 2019:

It can, but it would be wasteful; see detached comment.

in src/rpc/blockchain.cpp:174 in 9119244047 outdated

160@@ -160,6 +161,12 @@ UniValue blockToJSON(const CBlock& block, const CBlockIndex* tip, const CBlockIn
161         result.pushKV("previousblockhash", blockindex->pprev->GetBlockHash().GetHex());
162     if (pnext)
163         result.pushKV("nextblockhash", pnext->GetBlockHash().GetHex());
164+    if (g_signet_blocks) {
165+        std::vector<uint8_t> signet_commitment;
166+        if (block.GetWitnessCommitmentSection(SIGNET_HEADER, signet_commitment)) {
167+            result.pushKV("signet-solution", HexStr(signet_commitment));

jtimon commented at 2:51 pm on August 22, 2019:

it would be nice to also show the challenge in getblockchaininfo.

kallewoof commented at 4:59 am on August 23, 2019:

True! Adding.

jtimon commented at 2:54 pm on August 22, 2019: contributor

I still want to do more review and testing, but left some comments already.

kallewoof force-pushed on Aug 23, 2019

kallewoof commented at 5:15 am on August 23, 2019: member

Moved g_signet_blocks bool into consensus parameters.

However, moving g_signet_blockscript would require the params.h file to now #include <script/script.h> to get the CScript, which seems wasteful.

Alternatively I could make it a std::vector<unsigned char> in the consensus params, but then every time it’s used, it would have to be converted, so you’d get CScript(consensus.signet_blockscript.begin(), consensus.signet_blockscript.end()) sprinkled all over which doesn’t seem ideal.

kallewoof force-pushed on Aug 23, 2019

kallewoof force-pushed on Aug 28, 2019

jtimon commented at 3:53 pm on August 28, 2019: contributor

I think including script in consensus/params.h should be fine. I’m not sure I understand “wasteful” in this context. Is going to “waste” exactly the same memory.

But, if not, can we at least put it in chainparams?

On Fri, Aug 23, 2019, 07:18 kallewoof notifications@github.com wrote:

Moved g_signet_blocks bool into consensus parameters.

However, moving g_signet_blockscript would require the params.h file to now #include <script/script.h> to get the CScript, which seems wasteful.

Alternatively I could make it a std::vector in the consensus params, but then every time it’s used, it would have to be converted, so you’d get CScript(consensus.signet_blockscript.begin(), consensus.signet_blockscript.end()) sprinkled all over which doesn’t seem ideal.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/16411?email_source=notifications&email_token=AAHWGSV4TRS4YXT62FMOW4TQF5XLJA5CNFSM4IEW75LKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD47EPWA#issuecomment-524175320, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHWGSSCWTNGVH6XKSSEVXDQF5XLJANCNFSM4IEW75LA .

kallewoof force-pushed on Aug 29, 2019

kallewoof commented at 6:45 am on August 29, 2019: member

@jtimon

But, if not, can we at least put it in chainparams?

I mean, yes, we could totally do that, but I’m not sure which is better here to be honest. Putting it in chainparams means it’s close to the other stuff, which is good. But keeping it in signet.h means isolating signet stuff more from the rest of the code, which is also good.

in src/rpc/mining.cpp:1100 in b3896d51c2 outdated

1046+        if (!block.GetWitnessCommitmentSection(SIGNET_HEADER, signet_commitment)) {
1047+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Block has no signet commitment; please sign it first");
1048+        }
1049+    } else {
1050+        auto bin = ParseHexV(request.params[0], "challenge");
1051+        block = CreateSignetGenesisBlock(CScript(bin.begin(), bin.end()));

jtimon commented at 6:18 pm on September 7, 2019:

Why do we need to mine the genesis block, isn’t it easier to just not validate the genesis block beyond its hash defined in chainparams?

kallewoof commented at 5:39 am on September 8, 2019:

#16630 (comment)

jtimon commented at 9:24 pm on October 8, 2019:

With #17037 we wouldn’t need to grind the genesis block, and it is smaller than #8994 .

jtimon commented at 6:19 pm on September 7, 2019: contributor

I think it’s more consistent in chainparams, but I can see your point. Both options are valid, that’s just what I prefer.

in src/chainparams.cpp:275 in 10fac77f49 outdated

277+            genesis_nonce = 621297;
278+            vSeeds.push_back("178.128.221.177");
279+            vSeeds.push_back("2a01:7c8:d005:390::5");
280+            vSeeds.push_back("ntv3mtqw5wt63red.onion:38333");
281+        } else {
282+            if (!args.IsArgSet("-signet_blockscript")) {

jtimon commented at 8:07 pm on September 7, 2019:

This will never trigger as it has already been checked and handled. It can be removed.

in src/chainparams.cpp:273 in 10fac77f49 outdated

280+            vSeeds.push_back("ntv3mtqw5wt63red.onion:38333");
281+        } else {
282+            if (!args.IsArgSet("-signet_blockscript")) {
283+                throw std::runtime_error(strprintf("%s: -signet_blockscript is mandatory for signet networks", __func__));
284+            }
285+            if (args.GetArgs("-signet_blockscript").size() != 1) {

jtimon commented at 8:08 pm on September 7, 2019:

Why not just get the arg as a string?

kallewoof commented at 11:01 am on September 10, 2019:

I figured it was good to check there weren’t multiple blockscripts provided.

in src/chainparams.cpp:291 in aef5294c81 outdated

279+        }
280+
281+        LogPrintf("SigNet with block script %s\n", gArgs.GetArgs("-signet_blockscript")[0]);
282+
283+        strNetworkID = "signet";
284+        consensus.signet_blocks = true;

jtimon commented at 9:35 pm on September 7, 2019:

Can you set it to false in the others like regtest does with fPowNoRetargeting ?

kallewoof commented at 11:04 am on September 10, 2019:

OK!

jtimon commented at 5:35 pm on September 8, 2019: contributor

Created https://github.com/kallewoof/bitcoin/pull/9 with code solving most nits if not all. Also created https://github.com/jtimon/bitcoin/pull/12 to see how signet and custom chains could be combined best. Comments welcomed on any of them, and hopefully you will pick things from 9. The last commit requires a restart of the default network for it changes its genesis block.

kallewoof force-pushed on Sep 9, 2019

DrahtBot added the label Needs rebase on Sep 10, 2019

kallewoof force-pushed on Sep 10, 2019

kallewoof commented at 11:07 am on September 10, 2019: member

@jtimon Sorry for lack of responsiveness. I’ve addressed your nits separately and will compare to your branch.

in src/chainparamsbase.cpp:27 in 93dd0a1e62 outdated

23@@ -23,6 +24,11 @@ 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);

MarcoFalke commented at 11:23 am on September 10, 2019:

in commit 93dd0a1e62b019a72657f2abd60a2afc3d0702ba:

Can be removed, as this is redundant to -chain=signet

kallewoof commented at 11:31 am on September 10, 2019:

Isn’t it good to be able to do just -signet?

DrahtBot removed the label Needs rebase on Sep 10, 2019

jtimon commented at 7:43 pm on September 10, 2019: contributor

No worries, most of the commits in the branch are fixes to my own nits any way in case you find it convenient to squash them or something.

Regarding #16630 (comment) @MarcoFalke I strongly disagree. the only reason we need to have rpc functionality to grind genesis blocks and the only reason this patch needs the signet_nonce parameter is validating the pow in the genesis block.

There’s little reason to validate anything in the genesis block, for arguably the blocks is valid by definition and it’s itself a consensus rule (the sencond block must contain its hash). But philosophical questions aside, the changes in the consensus code are quite simple and innocuous. Not the way they’re written in #16630 though, for it has a performance penalty for other valid blocks. The way it is written in #9102 there’s no performance penalty at all unless the block is invalid, which I think it’s fine, since the penalty should be small anyway, I haven’t benchmarked it but it’s just comparing 2 hashes right before logging an error.

Sorry for the rant, but I really dislike the -signet_nonce argument. See https://github.com/kallewoof/bitcoin/pull/9/commits/010cca031dfb890f92ff78c3bf6a45326ae38edf

in src/chainparams.cpp:43 in 93dd0a1e62 outdated

39@@ -38,6 +40,12 @@ static CBlock CreateGenesisBlock(const char* pszTimestamp, const CScript& genesi
40     return genesis;
41 }
42 
43+static CBlock CreateGenesisBlock(const char* pszTimestamp, const CScript& genesisOutputScript, uint32_t nTime, uint32_t nNonce, uint32_t nBits, int32_t nVersion, const CAmount& genesisReward)

jtimon commented at 10:44 pm on September 10, 2019:

This function is only called from the next one and can be inlined. The change in the previous one is, I think all you need for your own CreateSignetGenesisBlock, which I would also prefer at the top (or at least before SigNetParams) in case it turns into a static function in the future. Really small nits.

kallewoof commented at 4:42 am on September 11, 2019:

I don’t think CreateSignetGenesisBlock will be static anytime soon, since it is also used in mining.cpp but I can definitely inline this one.

jtimon commented at 12:59 pm on September 11, 2019:

Well, it can only be static if we stop supporting grinding the genesis block because the genesis block’s pow is not checked anymore and we don’t need it. So I guess it depends on the other discussion. If we end up doing that, we won’t need to move the function later. But it’s a very minor nit, feel free to ignore.

kallewoof commented at 2:14 am on October 1, 2019:

Good point. If we end up reversing the grinding approach I will try to remember to address this.

kallewoof commented at 9:11 am on October 1, 2019:

One part about skipping genesis PoW that shouldn’t be forgotten is that this doesn’t just mean code changes in Bitcoin Core, but in every other software that checks the blockchain POW, e.g. btcd.

jtimon commented at 8:49 pm on October 8, 2019:

well, they will need changes to support chains whose genesis block doesn’t passs pow anyway, but yeah

kallewoof force-pushed on Sep 11, 2019

DrahtBot added the label Needs rebase on Sep 26, 2019

kallewoof force-pushed on Sep 27, 2019

DrahtBot removed the label Needs rebase on Sep 27, 2019

in src/primitives/block.h:156 in f5291323b0 outdated

151+    /**
152+     * Attempt to add or update the data for the section with the given header in the witness commitment of this block.
153+     *
154+     * This operation may fail and return false, if no witness commitment exists upon call time. Returns true on success.
155+     */
156+    bool SetWitnessCommitmentSection(const uint8_t header[4], const std::vector<uint8_t> data);

practicalswift commented at 3:20 pm on September 29, 2019:

data could be const ref?

kallewoof commented at 2:16 am on October 1, 2019:

I think I meant to do that, yeah. Thanks, fixed!

in src/primitives/block.h:161 in f5291323b0 outdated

156+    bool SetWitnessCommitmentSection(const uint8_t header[4], const std::vector<uint8_t> data);
157+
158+    /**
159+     * The tx based equivalent of the above.
160+     */
161+    static bool SetWitnessCommitmentSection(CMutableTransaction& tx, const uint8_t header[4], const std::vector<uint8_t> data);

practicalswift commented at 3:20 pm on September 29, 2019:

data could be const ref?

in src/chainparams.cpp:274 in f5291323b0 outdated

265@@ -246,13 +266,101 @@ class CTestNetParams : public CChainParams {
266     }
267 };
268 
269+/**
270+ * SigNet
271+ */
272+class SigNetParams : public CChainParams {
273+public:
274+    SigNetParams(const ArgsManager& args) {

practicalswift commented at 3:21 pm on September 29, 2019:

Single parameter ctor could be explicit?

in src/primitives/block.cpp:51 in f5291323b0 outdated

46+    }
47+    result.clear();
48+    return false;
49+}
50+
51+bool CBlock::SetWitnessCommitmentSection(CMutableTransaction& mtx, const uint8_t header[4], const std::vector<uint8_t> data)

practicalswift commented at 3:21 pm on September 29, 2019:

data const ref?

in src/primitives/block.cpp:87 in f5291323b0 outdated

82+    }
83+    mtx.vout[cidx].scriptPubKey = result;
84+    return true;
85+}
86+
87+bool CBlock::SetWitnessCommitmentSection(const uint8_t header[4], const std::vector<uint8_t> data)

practicalswift commented at 3:21 pm on September 29, 2019:

Same here :)

in src/rpc/mining.cpp:1074 in f5291323b0 outdated

 995+    } else {
 996+        util::insert(coinbase_script, ParseHexV(str, "coinbase_script"));
 997+    }
 998+
 999+    auto block = BlockAssembler(Params()).CreateNewBlock(coinbase_script)->block;
1000+    unsigned int extra_nonce = 0;

practicalswift commented at 3:22 pm on September 29, 2019:

The scope for extra_nonce could be reduced.

kallewoof commented at 2:31 am on October 1, 2019:

It doesn’t feel cleaner to put it inside the lock brackets as those are meant to do the lock, not limit scope.

in src/wallet/rpcwallet.cpp:4178 in f5291323b0 outdated

4173+
4174+    auto signet_hash = GetSignetHash(block);
4175+
4176+    CScript blockscript(g_signet_blockscript.begin(), g_signet_blockscript.end());
4177+
4178+    std::vector<uint8_t> sigdata;

practicalswift commented at 3:22 pm on September 29, 2019:

sigdata not used? :)

in contrib/signet/addtxtoblock.py:31 in f5291323b0 outdated

26+                                        usage='%(prog)s [addtxtoblock options] [bitcoin block file] [tx file] [fee]',
27+                                        description=__doc__,
28+                                        epilog='''Help text and arguments:''',
29+                                        formatter_class=argparse.RawTextHelpFormatter)
30+    # parser.add_argument('--filter', help='filter scripts to run by regular expression')
31+    args, unknown_args = parser.parse_known_args()

practicalswift commented at 3:25 pm on September 29, 2019:

Idiomatic alternative which signals that args is intentionally unused: _, unknown_args = parser.parse_known_args() :)

in test/functional/mining_signet.py:49 in f5291323b0 outdated

44+
45+    def skip_test_if_missing_module(self):
46+        self.skip_if_no_wallet()
47+
48+    def generate(self, node, count):
49+        for i in range(count):

practicalswift commented at 3:26 pm on September 29, 2019:

Nit: Same here: could use _.

in test/functional/mining_signet.py:48 in f5291323b0 outdated

43+        self.extra_args = [shared_args, shared_args]
44+
45+    def skip_test_if_missing_module(self):
46+        self.skip_if_no_wallet()
47+
48+    def generate(self, node, count):

practicalswift commented at 3:27 pm on September 29, 2019:

Nit: This could be a function instead of a method.

in test/functional/mining_signet.py:61 in f5291323b0 outdated

56+        node = self.nodes[0]
57+
58+        # give the privkey to node 1 so it can sign
59+        node.importprivkey(private_key)
60+        self.log.info('Imported network private key')
61+        self.log.info('address: %s, privkey: %s' % (address, node.dumpprivkey(address)))

practicalswift commented at 3:28 pm on September 29, 2019:

Instead of string format arguments use logging function parameters when using self.log.info :)

kallewoof commented at 2:38 am on October 1, 2019:

You mean % (...) → , ...? OK.

laanwj added the label Feature on Sep 30, 2019

kallewoof force-pushed on Oct 1, 2019

kallewoof force-pushed on Oct 2, 2019

carnhofdaki commented at 2:34 pm on October 8, 2019: contributor

No matter how far in syncing signet, it always says progress=1.000000.

0...
12019-10-08T14:32:08Z UpdateTip: new best=00000098714cd57f96b1bb4bdec02a869cb3269351bb2e110d56ea480e9a9e41 height=3140 version=0x30000000 log2_work=32.087826 tx=3152 date='2019-09-09T00:13:04Z' progress=1.000000 cache=0.4MiB(3155txo)
22019-10-08T14:32:08Z UpdateTip: new best=0000014e412d4d959c4d397cdd685d66cd32df7842984f490d493f57cdd031ac height=3141 version=0x30000000 log2_work=32.088886 tx=3153 date='2019-09-09T00:22:06Z' progress=1.000000 cache=0.4MiB(3156txo)
32019-10-08T14:32:08Z UpdateTip: new best=0000031045f7a9f625ea845ace16fe18153bbcfaf2445951df96c37d427a34a5 height=3142 version=0x30000000 log2_work=32.089945 tx=3154 date='2019-09-09T00:31:07Z' progress=1.000000 cache=0.4MiB(3157txo)
42019-10-08T14:32:08Z UpdateTip: new best=0000044ee1b6e1b471d28f3f32e4c92deb9828efc2ab5948e76ad70c753ad7c8 height=3143 version=0x30000000 log2_work=32.091004 tx=3155 date='2019-09-09T00:40:09Z' progress=1.000000 cache=0.4MiB(3158txo)
52019-10-08T14:32:08Z UpdateTip: new best=000003c670e58d4876da8ff3f21d12efd70984e30eb7f69e74c8a7754e5f4657 height=3144 version=0x30000000 log2_work=32.092062 tx=3156 date='2019-09-09T00:49:12Z' progress=1.000000 cache=0.4MiB(3159txo)

I do everything according to Getting Started section of https://en.bitcoin.it/wiki/Signet#Fetch_and_compile_signet

carnhofdaki commented at 3:20 pm on October 8, 2019: contributor

Also, errors like this are being shown on the console (yes, I am not running as daemon, but rather in tmux):

02019-10-08T15:13:23Z ERROR: AcceptBlockHeader: prev block not found
12019-10-08T15:13:26Z ERROR: AcceptBlockHeader: prev block not found

Let me know if it is proper that I am writing it here in this pull request.

in src/chainparams.cpp:43 in c584956573 outdated

39@@ -38,6 +40,12 @@ static CBlock CreateGenesisBlock(const char* pszTimestamp, const CScript& genesi
40     return genesis;
41 }
42 
43+static inline CBlock CreateGenesisBlock(const char* pszTimestamp, const CScript& genesisOutputScript, uint32_t nTime, uint32_t nNonce, uint32_t nBits, int32_t nVersion, const CAmount& genesisReward)

jtimon commented at 9:00 pm on October 8, 2019:

This is only called from the next CreateGenesisBlock, there’s no need for a new function.

in src/chainparams.cpp:70 in c584956573 outdated

66 }
67 
68+CBlock CreateSignetGenesisBlock(const CScript& block_script, uint32_t block_nonce)
69+{
70+    CHashWriter h(SER_DISK, 0);
71+    h << block_script;

jtimon commented at 9:05 pm on October 8, 2019:

Sorry to reiterate this. If it was answered elsewhere I missed it. Could we add the following?

0    h << strNetworkID;

I know it requires a network restart for the current signet network, but I think that should be alright to restart the network until the PR is merged. Even if we keep mining genesis blocks, this should make it easier to create a new signet chain with a different genesis block hash, even if the -signet_blockscript is the same.

Also, see #17037 for more context.

kallewoof commented at 5:02 am on October 10, 2019:

I agree adding network ID is a good idea. I’d like to not restart the network until things are more finalized, but I will put this on the list of things to change for the next reset.

in src/chainparamsbase.cpp:30 in c584956573 outdated

23@@ -23,6 +24,12 @@ 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_genesisnonce", "Nonce value for use in genesis block to satisfy proof of work requirement", ArgsManager::ALLOW_INT, OptionsCategory::CHAINPARAMS);
30+    gArgs.AddArg("-signet_hrp", "Human readable part of bech32 address (suffixed by \"sb\"; default = \"\")", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
31+    gArgs.AddArg("-signet_enforcescript", "Blocks must satisfy the given script to be considered valid (this replaces -signet_blockscript, and is used for opt-in-reorg mode)", ArgsManager::ALLOW_STRING, OptionsCategory::CHAINPARAMS);
32+    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);

jtimon commented at 9:18 pm on October 8, 2019:

The hrp and seeds arguments aren’t signet specific necessarily. You could drop the ‘signet’ prefix and make them configurable for regtest too. See #17037 for potential synergies. You could also make SigNetParams extend CRegTestParams for more code reusability.

kallewoof commented at 5:05 am on October 10, 2019:

That seems sensible in #17037, but there is literally no reason outside signet for these, here. I don’t think people will be too upset if we rename these a few months after merge, if worse comes to worst.

in src/qt/networkstyle.cpp:22 in c584956573 outdated

18@@ -19,7 +19,8 @@ static const struct {
19 } network_styles[] = {
20     {"main", QAPP_APP_NAME_DEFAULT, 0, 0},
21     {"test", QAPP_APP_NAME_TESTNET, 70, 30},
22-    {"regtest", QAPP_APP_NAME_REGTEST, 160, 30}
23+    {"regtest", QAPP_APP_NAME_REGTEST, 160, 30},

jtimon commented at 9:20 pm on October 8, 2019:

I forgot this ,, this +1-1 is my fault. My apologies.

in src/chainparams.cpp:315 in c584956573 outdated

324+        pchMessageStart[3] = 0x6a;
325+        nDefaultPort = 38333;
326+        nPruneAfterHeight = 1000;
327+
328+        genesis = CreateSignetGenesisBlock(g_signet_blockscript, genesis_nonce);
329+        consensus.hashGenesisBlock = genesis.GetHash();

jtimon commented at 0:06 am on October 9, 2019:

why not add the following here?

0        consensus.hashGenesisBlock = genesis.GetHash();
1        checkpointData = {
2            {
3                {0, consensus.hashGenesisBlock},
4            }
5        };

kallewoof commented at 5:06 am on October 10, 2019:

Does it change anything? (Actual question; I assumed it didn’t, except for blocks > height 0)

jtimon commented at 11:24 pm on October 23, 2019:

Well, I was thinking more on consistency with regtest, but functionally, I’m actually not sure it does anything (here or in regtest) either.

carnhofdaki commented at 1:05 pm on October 9, 2019: contributor

@kallewoof is signet meant to overwrite file blk00000.dat and not have any index? How is the explorer at https://explorer.bc-2.jp possible then?

kallewoof commented at 5:10 am on October 10, 2019: member

@carnhofdaki Thanks for testing! Comments below.

No matter how far in syncing signet, it always says progress=1.000000.

I’m confused. My bitcoin full node running on mainnet has this

02019-10-10T04:59:39Z UpdateTip: new best=0000000000000000000c41823b84f6273e2bf59648cfa7187eb2919213643a3d height=598693 version=0x20800000 log2_work=91.193405 tx=463424821 date='2019-10-10T04:58:54Z' progress=1.000000 cache=21.4MiB(60544txo) warning='45 of last 100 blocks have unexpected version'

so the behavior seems ordinary to me. What’s the problem?

2019-10-08T15:13:23Z ERROR: AcceptBlockHeader: prev block not found

This is because there are a couple of other signets running that have not properly removed the seed node parameter, so they are trying to connect to incompatible nodes (their genesis block is different, so in the end they get disconnected).

It’s a nuisance, but not a big problem.

is signet meant to overwrite file blk00000.dat and not have any index? How is the explorer at https://explorer.bc-2.jp possible then?

SIgnet works exactly like mainnet/testnet/regtest in regards to file I/O. By index, do you mean a transaction index? You need to do txindex=1 in your bitcoin.conf for that to appear. If you did, and it still isn’t appearing, do let me know. :)

kallewoof commented at 5:11 am on October 10, 2019: member

@jtimon Thanks for all the feedback/suggestions. I’m trying to work through them, but am a bit swamped at work right now so it may be a bit slower than normal. Will get back to you though.

carnhofdaki commented at 7:30 am on October 10, 2019: contributor

@kallewoof You are welcome! See my comments below.

I’m confused. My bitcoin full node running on mainnet has this

02019-10-10T04:59:39Z UpdateTip: new best=0000000000000000000c41823b84f6273e2bf59648cfa7187eb2919213643a3d height=598693 version=0x20800000 log2_work=91.193405 tx=463424821 date='2019-10-10T04:58:54Z' progress=1.000000 cache=21.4MiB(60544txo) warning='45 of last 100 blocks have unexpected version'

so the behavior seems ordinary to me. What’s the problem?

What you are showing is an actually synced bitcoin mainnet node. When you start syncing bitcoin mainnet without having any blocks, it will start with progress=0.000000 and slowly add there according to block index number devided by total blocks currently (and time, this is not an exact formula, just to get an idea).

2019-10-08T15:13:23Z ERROR: AcceptBlockHeader: prev block not found

This is because there are a couple of other signets running that have not properly removed the seed node parameter, so they are trying to connect to incompatible nodes (their genesis block is different, so in the end they get disconnected).

It’s a nuisance, but not a big problem.

I see.

is signet meant to overwrite file blk00000.dat and not have any index? How is the explorer at https://explorer.bc-2.jp possible then?

SIgnet works exactly like mainnet/testnet/regtest in regards to file I/O. By index, do you mean a transaction index? You need to do txindex=1 in your bitcoin.conf for that to appear. If you did, and it still isn’t appearing, do let me know. :)

I will re-run with txindex=1 but what I mean here is that when I run a signet full node, even after full sync to the block 8002, I have only file blk00000.dat with has last modification date set to when the last block was received.

Edit: ^^ this blk00000.dat issue is totally a non-issue. I misunderstood what is going on. There are not enough data on signet yet for other files to appear.

carnhofdaki commented at 6:37 pm on October 10, 2019: contributor

@kallewoof So I was playing more.

See this anomaly - it does not add up: https://explorer.bc-2.jp/tx/7bc98cb0298233bbeb8d43deebc879626c3403add1ebbf443c44a31576283aff

Edit: Shortened. Sorry for spam.

See block 8174

Edit: Finally I see the last zero in 8.999978 is omitted and that’s why 8.9999769 seemed to me that it does not add up.

carnhofdaki commented at 12:08 pm on October 11, 2019: contributor

Testing c584956573b553f33771b7d853e2ccf8f033d29a (kallewoof/bitcoin@signet) and 5cced1d78ee96b597dce23cff81f3ca991671fdf (kallewoof/bitcoin@signet-0.18) on ARM, Ubuntu 19.04, but berkleydb and boost are built from bitcoin/depends and installed to /usr (though not related to my issues).

I have issues with running a custom signet always ending on the last step - generating a block and signing it where I get:

0error code: -25
1error message:
2could not produce a signature -- do you have the private key(s)?

On the bitcoind side when I try to importprivkey I see this:

0[default wallet] Already have key with pubkey $PUBKEY, skipping
1[default wallet] Already have script <some_other_script>, skipping

Where the second line appears even on the first importprivkey $PRIVKEY call. <some_other_script> is a value different from any of the values referred by the wiki article

The same happens using the version recommended by wiki article - i.e. from the signet-0.18 branch of https://github.com/kallewoof/bitcoin.git

carnhofdaki commented at 10:58 am on October 15, 2019: contributor

Concept ACK, though I am unsure if signet should be configurable at all. (via signet_blockscript for example)

It should just be a shared public network, an alternative (or replacement) of testnet. @NicolasDorier as of my testing, the configurable signet currently does not work. I will be happy if someone can prove me wrong, but the steps at Signet - Bitcoin Wiki currently do not lead me to a working custom signet.

in src/primitives/block.h:134 in c584956573 outdated

129+    /**
130+     * Get the vout index of the segwit commitment in the given coinbase transaction.
131+     *
132+     * Returns -1 if no witness commitment was found.
133+     */
134+    template<typename T> static inline int GetWitnessCommitmentIndex(const T& coinbase) {

MarcoFalke commented at 3:18 pm on October 15, 2019:

Why is this moved? I think the goal for primitives is to be data-only and don’t contain any helper logic, especially if they are consensus critical.

kallewoof commented at 0:19 am on October 24, 2019:

It seems very weird to keep this obviously CBlock-related functionality in some completely different place. I’m not sure about primitives and data-only logic. If people feel strongly, I can restore this. Should the other stuff (witness commitment section stuff) follow along into validation.cpp then, you think?

kallewoof commented at 0:20 am on October 24, 2019: member

@carnhofdaki Can you make a log file showing all the steps of you setting up a custom signet to you getting the error? (and accompanying debug.log file would be helpful too)

kallewoof force-pushed on Oct 24, 2019

DrahtBot added the label Needs rebase on Oct 24, 2019

kallewoof force-pushed on Oct 27, 2019

DrahtBot removed the label Needs rebase on Oct 27, 2019

carnhofdaki commented at 6:12 pm on October 29, 2019: contributor

@kallewoof See log and the script which produced it at https://gist.github.com/carnhofdaki/60edef577f637ef2dbf4d244e4e279c2

Feel free to ask any questions if you wonder about the commands in the script. Generally it is just following the wiki article but fully automated.

DrahtBot added the label Needs rebase on Oct 30, 2019

kallewoof force-pushed on Oct 30, 2019

DrahtBot removed the label Needs rebase on Oct 30, 2019

DrahtBot added the label Needs rebase on Nov 2, 2019

kallewoof force-pushed on Nov 3, 2019

DrahtBot removed the label Needs rebase on Nov 3, 2019

kallewoof commented at 2:18 am on November 4, 2019: member

@carnhofdaki Sorry for late response. Tried your script and, yeah, that looks off. Will have to look closer into what might be causing it.

DrahtBot added the label Needs rebase on Nov 6, 2019

carnhofdaki commented at 5:41 pm on November 10, 2019: contributor

@carnhofdaki Sorry for late response. Tried your script and, yeah, that looks off. Will have to look closer into what might be causing it.

Thank you for having a look at the script!

kallewoof force-pushed on Dec 9, 2019

kallewoof commented at 1:33 am on December 9, 2019: member

@carnesen Sorry for delay on looking at that script, work is keeping me away from everything atm. Hoping to get back into things in a week or two, though. Will look then.

kallewoof renamed this:
~~Signet support~~
BIP-325: Signet support
on Dec 9, 2019

carnesen commented at 3:06 am on December 9, 2019: contributor

@carnesen Sorry for delay on looking at that script, work is keeping me away from everything atm. Hoping to get back into things in a week or two, though. Will look then. @carnhofdaki I think this was meant for you (cc @kallewoof)

DrahtBot removed the label Needs rebase on Dec 9, 2019

kallewoof commented at 3:24 am on December 9, 2019: member

@carnesen Thanks. Yeah, it was.

fjahr commented at 12:24 pm on December 27, 2019: member

Concept ACK

Great work! I set up a custom signet a few months ago and found it very easy to do and it was running smoothly (followed the wiki article, not a review, code has changed a lot since then). However, I also discovered that (as is usually the case) coordinating other participants in that Signet was the hard part. So, I now think the main contribution of Signet will be the global signet with regularly scheduled fee spikes, attack scenarios etc. where the behavior of all kinds of upcoming PRs can be tested and measured on a regular basis, especially P2P behavior. But even if custom signets may be used less than I thought initially, I still see the value in them for upcoming softforks like taproot. Will try to review the code in the coming weeks.

Edit: If you were to schedule some events on the global signet now already that maybe would attract more contributors to deploy their own PRs to the global signet and give feedback here as well, a win-win. Just an idea :)

practicalswift commented at 9:13 pm on December 27, 2019: contributor

This PR has gotten four Concept ACKs and zero Concept NACKs if I’m counting correctly.

Please chime in: more Concept ACKs or Concept NACKs?

The benefits have been discussed extensively: what costs can we see?

Personally I think signet would be very valuable to have when testing.

kallewoof commented at 7:46 am on December 29, 2019: member

@carnhofdaki

@kallewoof See log and the script which produced it at https://gist.github.com/carnhofdaki/60edef577f637ef2dbf4d244e4e279c2

Feel free to ask any questions if you wonder about the commands in the script. Generally it is just following the wiki article but fully automated.

Sorry for the extremely late reply. I found the issue – you are setting LEN to the length of the hexadecimal string, rather than the length of the actual data. One fix:

0- LEN=$(printf $PUBKEY | wc -c)
1+ LENX2=$(printf $PUBKEY | wc -c)
2+ LEN=$((LENX2/2))

kallewoof commented at 7:49 am on December 29, 2019: member

@fjahr Thanks for the feedback! I was honestly dragged away from everything for the last couple of months, and am finally getting back to things now. Scheduling events on the global running signet is something I’ve been meaning to do for a long time. The code to do things like intentional reorgs and such is in place, but lacking a UI right now.

If you have any concrete ideas I’d love to hear them :) @practicalswift Thanks for the nudge! I will bring up bumping signet over to high priority from needs concept ACK on IRC.

fjahr commented at 12:17 pm on December 31, 2019: member

If you have any concrete ideas I’d love to hear them :)

I am not sure about priorities as I am not working on something where I need it right now, but some ideas could be interesting:

“larger” re-orgs
large numbers of transactions with rising fee rate, so that default sized mempools start evicting transactions
mining empty blocks
full blocks but transactions were not in the mempool
network hash-rate drop/spike

Ping @amitiuttarwar what would be helpful for your work on #16698?

laanwj moved this from the "Chasing Concept ACK" to the "Blockers" column in a project

DrahtBot added the label Needs rebase on Jan 2, 2020

kallewoof force-pushed on Jan 3, 2020

DrahtBot removed the label Needs rebase on Jan 3, 2020

kallewoof force-pushed on Jan 3, 2020

kallewoof commented at 1:05 pm on January 3, 2020: member

I have no idea why tests are failing… they aren’t, locally.

MarcoFalke commented at 1:15 pm on January 3, 2020: member

re-run ci

MarcoFalke closed this on Jan 3, 2020

MarcoFalke reopened this on Jan 3, 2020

kallewoof force-pushed on Jan 5, 2020

carnhofdaki commented at 1:51 pm on January 6, 2020: contributor

@kallewoof Thank you! #16411 (comment) It works well now!

kallewoof commented at 8:32 am on January 7, 2020: member

I think we can remove “Needs Conceptual Review” tag as well.

kallewoof force-pushed on Jan 11, 2020

kallewoof force-pushed on Jan 15, 2020

kallewoof commented at 8:02 am on January 15, 2020: member

Bumped copyright year. Note that Travis tests did not run, only Appveyor.

jtimon commented at 7:12 pm on January 15, 2020: contributor

Can we have the discussion on not checking the genesis’ block pow vs needing grind the genesis block again? Can anybody remind me what are the main arguments in favor of having to grind the genesis block?

kallewoof commented at 3:36 am on January 18, 2020: member

@jtimon I think we should discuss on the mailing list. I’ll send an email listing pros and cons.

Edit: Actually, I think this is contentious enough that I’d rather just get signet v 1.0 in as it is with the genesis nonce, and then later see about a signet 2.0 which shortcuts the genesis POW.

fanquake removed the label Needs Conceptual Review on Jan 20, 2020

kallewoof force-pushed on Jan 23, 2020

NicolasDorier commented at 5:45 am on January 24, 2020: contributor

I started talking with @kallewoof about signet, and I thought of something.

Problem

Software built on top of bitcoin core very often hard code and rely on the genesis block hash. However it seems that because of this, each signet will have a different genesis block. This mean that every times you use a different signet, you would need to rebuild every software on top of of bitcoin core. This is a friction that would prevent people from using signet.

Specific use case

For example, if a project using NBitcoin would decide to use another signet, the developer would need me to hardcode his new signet in my library then I need to push a new version of my library, then he would need to recompile his project to use this version and finally able to use the new signet. Moving back and forth between signet would require such kind of heavy coordination between many people involved.

Because in real code the genesis block never change, it is not wise either for NBitcoin to allow dynamic network parameters. This would push the complexity to the user of my library, and make his code a bit farther away from their mainnet.

Proposal

Instead of putting the challenge scriptPubKey in the genesis block, it should be put in the block just after.

The other advantage of it is that we don’t need to grind the genesis block to pass the PoW check everytimes somebody change the signet params.

kallewoof commented at 6:11 am on January 24, 2020: member

It would definitely simplify software since most stuff I’ve found has a hard coded genesis block.

The downside is that most light clients (anything that doesn’t also check the block signature) will run the risk of switching to the wrong chain!

kallewoof commented at 6:37 am on January 24, 2020: member

One partial solution to the problem would be to set the magic number (pchMessageStart) to be e.g. the first 4 bytes of sha256d(signet_challenge). There would be a 1 in 2^32 chance that two signets’ magic numbers would collide, but I think this is acceptable, as most SPV nodes will have trusted nodes they connect to for signet testing anyway.

Interested to hear people’s thoughts!

Sjors commented at 8:45 am on January 24, 2020: member

It sounds reasonable to have a fixed genesis block for all signets, with differentiation happening in the first block. Taking the thought a bit further, could you have an arbitrary height where the signet constraint activates (default 1)? That might make it easier to fork signets, so you can try new consensus features on an existing UTXO set.

The sha256d(signet_challenge) also sounds reasonable. You could write a test case with two different signets to demonstrate this.

jtimon commented at 8:54 pm on January 24, 2020: contributor

Instead of putting the challenge scriptPubKey in the genesis block, it should be put in the block just after.

But you need them to have different genesis blocks. For example, the genesis block is the chain_id in lightning networks. See, for example, my project in which I use 3 different regtest chains for lightning. I’m not using signet there, but it would be similar.

https://github.com/jtimon/multi-ln-demo/blob/master/multiln/chains.py

I guess that file is not very interesting, but different chains need different genesis blocks.

This mean that every times you use a different signet, you would need to rebuild every software on top of of bitcoin core.

Alternatively those projects can reimplement how to calculate the genesis hash from the challenge script. This alternative requires that the pow for genesis blocks isn’t checked so that there’s no need to grind the genesis block, which is desirable for simplicity anyway, and I am yet to hear a good argument against.

kallewoof force-pushed on Jan 30, 2020

0xB10C commented at 11:04 am on February 7, 2020: member

Syncing signet (both the default one and a custom one) reports an incorrect IBD progress while in IBD and after the IBD. The wrong progress can be seen in the debug.log and via getblockchaininfo and potentially causes confusion. Haven’t looked into why the calculation is wrong.

 0./bitcoin-cli -signet getblockchaininfo
 1{
 2  "chain": "signet",
 3  "blocks": 27004,
 4  "headers": 27004,
 5  "bestblockhash": "0000015194fa56d07f9cf8ce3c23e3e5ab64d68cfa06fa15e622b86336db10c0",
 6  "difficulty": 0.002338098747362749,
 7  "mediantime": 1581068861,
 8  "verificationprogress": 5.484926296056364e-05,
 9  "initialblockdownload": false,
10  "chainwork": "000000000000000000000000000000000000000000000000000000236405f47a",
11  "size_on_disk": 10652880,
12  "pruned": false,
13  "signet-blockscript": "512103ad5e0edad18cb1f0fc0d28a3d4f1f3e445640337489abb10404f2d1e086be43051ae",
14  "softforks": {
15  // shortend  
16  },
17  "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
18}

kallewoof commented at 11:54 am on February 7, 2020: member

@0xB10C Unfortunately this is not trivial to fix, because it depends on statistics manually put into the chainparams file (see e.g. the mainnet class a few pages up, where it has info about the number of transactions and such in the network). For a very-long-term signet, it might make sense to start adding those, but whenever the signet network switches to a new one, all of that would become redundant.

ajtowns commented at 5:31 am on March 5, 2020: member

But you need them to have different genesis blocks. For example, the genesis block is the chain_id in lightning networks. @jtimon per bolt-00 you don’t have to use the genesis block as the chain id, so I think it should be fine to use block 1 as the chain id for testing lightning on signet

kallewoof force-pushed on Mar 5, 2020

laanwj removed this from the "Blockers" column in a project

add witness commitment section support to primitive/block

This also moves GetWitnessCommitmentIndex out of validation.cpp into CBlock, with 3 lines affected (GetWitnessCommitmentIndex(block) -> block.GetWitnessCommitmentIndex()).

aeb3cd5647

kallewoof force-pushed on Mar 6, 2020

add simple signature support (checker) 68cd95a881

add simple signature support (creator) dff9e488cf

add signet basic support (signet.cpp) b90cfc0570

add signet chain and accompanying parameters 82e69c3cdf

qt: update QT to support signet network eacb99812f

consensus: add signet validation 6de22e6af5

i/o: make DecodeHexBlk able to read block hex from file 088a5252b1

rpc: add getnewblockhex command for obtaining an unsigned block 56a373cea1

rpc: add signblock command c4cad20a0f

rpc: include signet commitment in blockToJSON dictionary results 3a8073f2fb

rpc: refactor out grindBlock part from generateBlocks 81b69ab520

rpc: add grindblock command for generating POW on a signed signet block a7d76a5cf4

rpc/signet: show signet blockscript in 'getblockchaininfo' 055047c463

kallewoof force-pushed on Mar 6, 2020

test: add signet mining tests

Also adds a condition to test initialization code to not generate blocks for signet chains.

37269b19d2

contrib: add signet scripts (issuer, etc) 50c839e1d9

signet: hard-coded parameters for Signet Global Network IV (2020-03-05)

Running with -signet without any other parameters will use this network's parameters.

5dc6414b3d

util_tests.cpp update hash for hard coded chain merge 89e402443a

kallewoof closed this on Mar 6, 2020

kallewoof reopened this on Mar 6, 2020

DrahtBot commented at 7:07 pm on March 11, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Mar 11, 2020

jtimon commented at 9:43 pm on March 11, 2020: contributor

But you need them to have different genesis blocks. For example, the genesis block is the chain_id in lightning networks.

@jtimon per bolt-00 you don’t have to use the genesis block as the chain id, so I think it should be fine to use block 1 as the chain id for testing lightning on signet

Well, but in that case, you would need to hardcode the second block instead of the genesis block and you have the same problem @NicolasDorier complains about, no?

I understand that some use cases don’t need a chain_id or anything like that, and for those it is fine to have the same the same genesis hash and they don’t even care about the second block hash because they won’t hardcode that either. But for lightning, I think you need a chain_id.

Supporting multiple chains doesn’t require external software to hardcode every new chain though, that’s just a design decision. Just like bitcoin core generates the hash in a deterministic way, other software can too. Instead of implementing only a few hardcoded signets or only the default one, external software could implement them all possible signets at once, just like bitcoin core does.

For this, it would help to not mine the genesis block and thus not need to handle the nonce as configurable anywhere, but that’s orthogonal. Sorry to insist, but can we discuss what are the advantages of grinding the genesis block again?

Back to @NicolasDorier ’s proposal of committing the challenge to the second block instead of the first one, I’m all fine with that, it’s just that I also want to generate chains that do have different genesis blocks when I want to. This could be done, for example, by hashing the network name. People wanting the same genesis hash could all just leave the default “signet” while others could customize different names, resulting in different genesis blocks, like in #17037

This again would be simpler if the genesis block wasn’t mined.

Or perhaps we could even allow both having the challenge being committed on the genesis block or on the second one as a configuration option?

kallewoof commented at 6:09 am on March 12, 2020: member

I just want to add to this that, with the hard coded genesis block, all software will still need to re-generate the magic number (message header / message start / whatever you wanna call it) when they switch signets. I think this is significantly easier than a dynamic genesis block hash, but it’s still not “plug-and-play” to switch to a different signet. Ping @NicolasDorier.

kallewoof commented at 3:39 am on March 24, 2020: member

Closing in favor of #18267.

kallewoof closed this on Mar 24, 2020

laanwj referenced this in commit 8c5f68118c on Sep 21, 2020

sidhujag referenced this in commit dc85269389 on Sep 22, 2020

KolbyML referenced this in commit 9dfef518f7 on Jan 17, 2021

KolbyML referenced this in commit ebf48e94dd on Jan 17, 2021

PastaPastaPasta referenced this in commit e197f976e1 on Jan 22, 2021

DrahtBot locked this on Feb 15, 2022

BIP-325: Signet support #16411

Conflicts

Problem

Specific use case

Proposal