Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Signet support #16411

Open
wants to merge 17 commits into
base: master
from

Conversation

@kallewoof
Copy link
Member

commented Jul 18, 2019

This PR implements bitcoin/bips#803 -- Signet support.

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.

See also: https://en.bitcoin.it/wiki/Signet

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.

@junderw

This comment has been minimized.

Copy link

commented Jul 18, 2019

Brainstorming some things we should consider:

1. Does adding this put anyone at risk?
2. Does adding this somehow hurt Bitcoin's code base?
3. Does adding this require some long term maintenance / increase the weight on devs for support in the future?
  1. 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.)
  2. I would say no. But that would need more in-depth technical review.
  3. 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

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2019

@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

This comment has been minimized.

Copy link

commented Jul 18, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2019

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

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16704 (Don't warn about activated buried BIP 9 deployments by achow101)
  • #16680 (Preparations for more testchains by jtimon)
  • #16653 (script: add simple signature support (checker/creator) by kallewoof)
  • #16378 ([WIP] The ultimate send RPC by Sjors)
  • #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
  • #16324 (Get cs_main out of the critical path in ProcessMessages by TheBlueMatt)
  • #16323 (Call ProcessNewBlock() asynchronously by TheBlueMatt)
  • #16279 (Return the AcceptBlock CValidationState directly in ProcessNewBlock by TheBlueMatt)
  • #15934 (Separate settings merging from parsing by ryanofsky)
  • #13751 (Utils and libraries: Drops the boost/algorithm/string/split.hpp dependency by l2a5b1)
  • #8994 (Testchains: Introduce custom chain whose constructor... by jtimon)

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

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

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.

@kallewoof kallewoof referenced this pull request Jul 18, 2019

@laanwj laanwj added this to Chasing Concept ACK in High-priority for review Jul 18, 2019

@kallewoof kallewoof force-pushed the kallewoof:signet branch 5 times, most recently from 8789b74 to 587526d Jul 19, 2019

if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
// We skip POW checks for genesis block
if (block.GetHash() != consensusParams.hashGenesisBlock) {

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Jul 19, 2019

Member

I would move this test inside CheckProofOfWork

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jul 19, 2019

Author Member

I need this if block for CheckBlockSolution too, though.

bitcoin/src/validation.cpp

Lines 945 to 952 in 587526d

// We skip POW checks for genesis block
if (block.GetHash() != consensusParams.hashGenesisBlock) {
// Check the header
if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());
if (g_signet_blocks && !CheckBlockSolution(block, consensusParams)) return false; /* function calls error(..) */
}

if (!CheckBlockHeader(block, state, consensusParams, fCheckPOW))
return false;
// We skip POW checks for genesis block
if (block.GetHash() != consensusParams.hashGenesisBlock) {

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Jul 19, 2019

Member

I would move this test inside CheckBlockHeader

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jul 19, 2019

Author Member

I need this if block for CheckBlockSolution too, though.

bitcoin/src/validation.cpp

Lines 2974 to 2982 in 587526d

// We skip POW checks for genesis block
if (block.GetHash() != consensusParams.hashGenesisBlock) {
// Check that the header is valid (particularly PoW). This is mostly
// redundant with the call in AcceptBlockHeader.
if (!CheckBlockHeader(block, state, consensusParams, fCheckPOW))
return false;
if (g_signet_blocks && fCheckPOW && !CheckBlockSolution(block, consensusParams)) return false;
}

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Jul 19, 2019

Member

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

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jul 19, 2019

Author Member

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

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

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

@kallewoof kallewoof force-pushed the kallewoof:signet branch from 587526d to 454650b Jul 20, 2019

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2019

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

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2019

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

@practicalswift

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Concept ACK

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

@kallewoof kallewoof referenced this pull request Jul 23, 2019

@kallewoof kallewoof force-pushed the kallewoof:signet branch 3 times, most recently from 43cfbfb to ac9da7d Jul 30, 2019


A script to generate one Signet block.

Syntax: `mkblock.sh <bitcoin-cli path> [<bitcoin-cli args>]`

This comment has been minimized.

Copy link
@MarcoFalke

MarcoFalke Jul 30, 2019

Member

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?

This comment has been minimized.

Copy link
@junderw

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

This comment has been minimized.

Copy link
@kallewoof

kallewoof Jul 31, 2019

Author Member

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

@Sjors

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

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

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@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

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

@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

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

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

@Sjors Sjors referenced this pull request Aug 19, 2019
8 of 8 tasks complete
@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

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?

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Aug 22, 2019

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

@jtimon
Copy link
Member

left a comment

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

@@ -968,6 +969,8 @@ bool ReadBlockFromDisk(CBlock& block, const FlatFilePos& pos, const Consensus::P
if (!CheckProofOfWork(block.GetHash(), block.nBits, consensusParams))
return error("ReadBlockFromDisk: Errors in block header at %s", pos.ToString());

if (g_signet_blocks && block.GetHash() != consensusParams.hashGenesisBlock && !CheckBlockSolution(block, consensusParams)) return false; /* function calls error(..) */

This comment has been minimized.

Copy link
@jtimon

jtimon Aug 22, 2019

Member

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

This comment has been minimized.

Copy link
@kallewoof

kallewoof Aug 23, 2019

Author Member

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

@@ -3036,6 +3039,8 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P
if (!CheckBlockHeader(block, state, consensusParams, fCheckPOW))
return false;

if (g_signet_blocks && fCheckPOW && block.GetHash() != consensusParams.hashGenesisBlock && !CheckBlockSolution(block, consensusParams)) return false;

This comment has been minimized.

Copy link
@jtimon

jtimon Aug 22, 2019

Member

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.

This comment has been minimized.

Copy link
@kallewoof

kallewoof Aug 23, 2019

Author Member

CheckBlockSolution requires the block, not just the header.

Moving return to own line for readability.

#include <util/system.h>

bool g_signet_blocks = false;
CScript g_signet_blockscript;

This comment has been minimized.

Copy link
@jtimon

jtimon Aug 22, 2019

Member

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

This comment has been minimized.

Copy link
@kallewoof

kallewoof Aug 23, 2019

Author Member

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

if (g_signet_blocks) {
std::vector<uint8_t> signet_commitment;
if (block.GetWitnessCommitmentSection(SIGNET_HEADER, signet_commitment)) {
result.pushKV("signet-solution", HexStr(signet_commitment));

This comment has been minimized.

Copy link
@jtimon

jtimon Aug 22, 2019

Member

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

This comment has been minimized.

Copy link
@kallewoof

kallewoof Aug 23, 2019

Author Member

True! Adding.

@kallewoof kallewoof force-pushed the kallewoof:signet branch 2 times, most recently from d6a8a68 to c15b1ff Aug 23, 2019

@kallewoof

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

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 added some commits Jun 28, 2019

signet: hard-coded parameters for Signet Global Network III (2019-08-19)
Running with -signet without any other parameters will use this network's parameters.
test: added signet mining tests
Also adds a condition to test initialization code to not generate blocks for signet chains.

@kallewoof kallewoof force-pushed the kallewoof:signet branch from 17bcd56 to 2b9db9a Aug 23, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.