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

NOMERGE: Support signet as parent chain #414

Open
wants to merge 6 commits into
base: elements-0.14.1
Choose a base branch
from

Conversation

jtimon
Copy link
Contributor

@jtimon jtimon commented Sep 21, 2018

signet has simpler signed blocks but no CT or any other extra feature from elements.

Suggestion to test this: #433

Dependencies:

@jtimon jtimon changed the title WIP: Support signet as parent chain WIP Support signet as parent chain Sep 21, 2018
@jtimon jtimon force-pushed the e14-parentchain-signet branch 2 times, most recently from 91059bf to 4380518 Compare September 21, 2018 07:21
@jtimon jtimon changed the title WIP Support signet as parent chain TOTEST Support signet as parent chain Sep 22, 2018
@jtimon jtimon changed the title TOTEST Support signet as parent chain TOTEST: Support signet as parent chain Sep 22, 2018
@jtimon
Copy link
Contributor Author

jtimon commented Sep 22, 2018

fwiw, this seems to be working in the explorer, see jtimon/elements-explorer@df65994
But it would still be good to reuse the feature_peg.py modifying it a little bit to test this, even if it's only locally like when the parent chain is regtest before merging this.

src/pow.cpp Outdated
{
if (g_solution_blocks) {
if (hash == params.hashGenesisBlock) return true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't need or want this here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would need to provide the signature for the genesis block otherwise, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is used to validate the parent chain. The sidechain doesn't validate the parent chain's genesis block (in fact, not even the parent chain should validate the genesis block, the genesis block is a consensus rule in itself and thus doesn't need to be validated, not when it's pow nor when it's blocksigned).

@jtimon jtimon force-pushed the e14-parentchain-signet branch 3 times, most recently from 0fd6752 to 1e8648a Compare September 27, 2018 00:23
@jtimon jtimon changed the title TOTEST: Support signet as parent chain Support signet as parent chain Sep 27, 2018
@jtimon
Copy link
Contributor Author

jtimon commented Sep 27, 2018

This can now be tested with:

python3 ./qa/pull-tester/rpc-tests.py feature_fedpeg --parent_type=signet --parent_binpath=/home/user/code/bitcoin/src/bitcoind

Assuming you're building https://github.com/kallewoof/bitcoin/commits/signet ( b4c9e78 ) on /home/user/code/bitcoin/.

The changes to feature_fedpeg shouldn't break other ways to call the tests:

python3 ./qa/pull-tester/rpc-tests.py feature_fedpeg
python3 ./qa/pull-tester/rpc-tests.py feature_fedpeg --parent_type=bitcoin --parent_binpath=/home/jt/data/programs/binaries/bitcoin-0.16.3/bin/bitcoind
python3 ./qa/pull-tester/rpc-tests.py feature_fedpeg --parent_type=bitcoin --parent_binpath=/home/jt/data/programs/binaries/bitcoin-0.17.0/bin/bitcoind

self.binary = self.options.parent_binpath if self.options.parent_binpath != "" else None
self.nodes.append(start_node(n, self.options.tmpdir, self.extra_args[n], binary=self.binary, chain=self.parent_chain))
if self.options.parent_type == 'signet':
self.nodes[n].importprivkey('8Hh8jNjkx1aSCgEk3iq9Vo2APZUSDQJVt3rJ2BRpb5Tavqb68vW')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this magic value related to the one in -signet_blockscript? Perhaps define them as variables somewhere earlier then together so that it's clear they are related.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will eventually go away, I assume, unless we agree on a global default signet (seems unlikely, at least for upstream to bitcoin/bitcoin).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we could generate the block signing script and the wif dynamically by starting a node, getnewaddress, dumpprivkey, etc beforehand and then starting the actual testing nodes with the resulting script.
This is just the wif for the public key corresponding to the script.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I was using for generating the p2pk sript and the corresponding wif to import:

jtimon@3a597eb

@@ -140,13 +153,21 @@ def test_pegout(self, parent_chain_addr, sidechain):
break
assert pegout_tested

# TODO Remove when signet supports generate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an upstream issue to track for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, all we have for now is bitcoin/bitcoin@master...kallewoof:signet where I'm commenting directly on the code.
Pinging @kallewoof since I can't remember if I suggested this to him.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intend to make a PR against bitcoin core. For now there is a PR against my repo here: kallewoof/bitcoin#4

* Contains a mapping of hash to signature data for each block header
* in signet networks.
*/
extern std::map<uint256,std::vector<uint8_t>> g_blockheader_payload_map;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this supposed to be like a global cache? It doesn't seem to get emptied ever.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An in-memory cache that, most importantly, doesn't inflate memory usage when signet feature is off.

This does mean an ever-growing set when on, yes. Wonder what cane be done with this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kallewoof thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's ever growing, yep. It could be cached since header sigs are stored along with headers. I haven't thought too much about it, as even a 1 million block chain (that's 2x the lifetime of bitcoin right now) would only have ~100 MB of RAM use.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also when it's a 11-of-15 multisig? Or even larger if this script doesn't have the same script limits as normal tx scripts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right you would basically store siglen + alpha per block. For a 15 multisig over ECDSA it'd get big quicker, I suspect, but I assume we will eventually switch to Schnorr multisigs long term.

@jtimon
Copy link
Contributor Author

jtimon commented Oct 6, 2018

Now we can review signet more easily with kallewoof/bitcoin#4

@jtimon
Copy link
Contributor Author

jtimon commented Oct 6, 2018

Needed rebase. Also, removed one TODO and its workaround in the tests since now generate works directly with signet.

@jtimon
Copy link
Contributor Author

jtimon commented Oct 6, 2018

Moved to kallewoof/bitcoin#5 and removed the special case for the p2sh test for signet.

@jtimon
Copy link
Contributor Author

jtimon commented Oct 9, 2018

Updated, now it doesn't have the wif and blockscript hardcoded but generated aumotatically

@jtimon
Copy link
Contributor Author

jtimon commented Oct 9, 2018

Fixed travis.

@stevenroose
Copy link
Member

tACK 23cb9ae with parent 2aca2a9 (kallewoof/signet-0.17)

@instagibbs
Copy link
Collaborator

Could we get travis testing against a signet parent binary?

@stevenroose
Copy link
Member

That would be ideal of course. Could be as simple as the Bitcoin extra Travis thing. But then there need to be some kind of "stable" signet release..

@instagibbs
Copy link
Collaborator

@stevenroose I'm simply reticent to merge something we can't be confident of stability of the protocol itself.

@kallewoof any plans on "finalizing" this, whatever that means to you? :) We clearly can adapt implementation later to later upgrades.

@stevenroose
Copy link
Member

Maybe when the elements-0.17 work is done, signet is "released" and we can download a stable binary. I wouldn't mind to keep this open. Or either way just have a custom-compiled signet binary in Travis.

@jtimon jtimon changed the title Support signet as parent chain NOMERGE: Support signet as parent chain Oct 16, 2018
@jtimon
Copy link
Contributor Author

jtimon commented Oct 16, 2018

Added a commit (not squashed yet) supposedly adapting the test to #433 as parent chain binary.

But even though everything else should have stayed the same, travis seems to be failing.
...it's not the first time assetdir.py randomly fails for me in elements-0.14, let me ask travis to try again...

@jtimon jtimon mentioned this pull request Nov 22, 2018
3 tasks
@instagibbs instagibbs added the needs port Needs backport to a different branch label Mar 25, 2019
@instagibbs
Copy link
Collaborator

@kallewoof have you or do you have intentions of releasing binaries for signet? We could at least get this ported to master for support/iteration.

@kallewoof
Copy link
Contributor

kallewoof commented Mar 28, 2019

@instagibbs Sorry for lack of activity. I am intending on proposing a BIP for signet in the near future. Things will be switched around a bit, I suspect, so no binaries yet.

Edit: to actually answer your question, I think having binaries for signet would be useful and will see about making those available. No timeframe at this point in time.

@instagibbs
Copy link
Collaborator

Ok just curious. Still interested at least in supporting as parent chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.14.1 needs port Needs backport to a different branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants