Search code, repositories, users, issues, pull requests...

Copy link

Contributor

@jtimon jtimon commented Sep 21, 2018 •

edited

Loading

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

Suggestion to test this: #433

Dependencies:

QA: feature_fedpeg.py: Remove re-implementation of sync_all QA: feature_fedpeg.py: Remove re-implementation of sync_all #421

@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

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.

jtimon

jtimon commented

Sep 22, 2018

src/pow.cpp Outdated

bool CheckBitcoinProof(const uint256& hash, unsigned int nBits, const Consensus::Params& params)

{

if (g_solution_blocks) {

if (hash == params.hashGenesisBlock) return true;

Copy link

Contributor Author

@jtimon jtimon Sep 22, 2018

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

@kallewoof kallewoof Oct 6, 2018

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

@jtimon jtimon Oct 6, 2018

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 (追記ここまで)

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

stevenroose

stevenroose reviewed

qa/rpc-tests/feature_fedpeg.py Outdated

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

Contributor

@stevenroose stevenroose Sep 27, 2018

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

@kallewoof kallewoof Sep 28, 2018

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

@jtimon jtimon Oct 4, 2018

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

@jtimon jtimon Oct 6, 2018

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

stevenroose

stevenroose reviewed

qa/rpc-tests/feature_fedpeg.py Outdated

break

assert pegout_tested

# TODO Remove when signet supports generate

Copy link

Contributor

@stevenroose stevenroose Sep 27, 2018

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

@jtimon jtimon Oct 4, 2018

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

@kallewoof kallewoof Oct 6, 2018

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

stevenroose

stevenroose reviewed

src/primitives/bitcoin/block.h

* 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

Contributor

@stevenroose stevenroose Sep 27, 2018

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

Contributor

@instagibbs instagibbs Sep 27, 2018

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

Contributor

@instagibbs instagibbs Sep 27, 2018

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

@kallewoof kallewoof Sep 28, 2018

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

Contributor

@stevenroose stevenroose Sep 28, 2018

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

@kallewoof kallewoof Oct 6, 2018

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.

Copy link

Contributor Author

jtimon commented Oct 6, 2018

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

@jtimon jtimon force-pushed the e14-parentchain-signet branch from 1e8648a to 9dcf7dd Compare

October 6, 2018 08:39

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 jtimon force-pushed the e14-parentchain-signet branch from bbc58b9 to 4d9ad55 Compare

October 6, 2018 14:55

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 jtimon force-pushed the e14-parentchain-signet branch from 4d9ad55 to f95af12 Compare

October 7, 2018 06:07

jtimon added 3 commits

October 9, 2018 09:55


 Backport signet block serialization


 Reuse block signing code for signet and elements

1f5e0a5


 Support signet as parent chain

c4ddcee

signed has simpler signed blocks but no CT or any other extra feature

@jtimon jtimon force-pushed the e14-parentchain-signet branch from f95af12 to 1518f6e Compare

October 9, 2018 08:49

Copy link

Contributor Author

jtimon commented Oct 9, 2018

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

jtimon added 2 commits

October 9, 2018 17:12


 QA: feature_fedpeg.py: Move from --parent_bitcoin to --parent_type

b3a28f2


 QA: Test support for signet as parent chain

23cb9ae

@jtimon jtimon force-pushed the e14-parentchain-signet branch from 1518f6e to 23cb9ae Compare

October 9, 2018 15:14

Copy link

Contributor Author

jtimon commented Oct 9, 2018

Fixed travis.

@stevenroose

Copy link

Contributor

stevenroose commented Oct 10, 2018

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

Copy link

Contributor

instagibbs commented Oct 12, 2018

Could we get travis testing against a signet parent binary?

@stevenroose

Copy link

Contributor

stevenroose commented Oct 16, 2018

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

Copy link

Contributor

instagibbs commented Oct 16, 2018

@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

Contributor

stevenroose commented Oct 16, 2018

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


 SQUASH: Adapt signet fedpeg test to ElementsProject#433

cc7d24f

@jtimon jtimon force-pushed the e14-parentchain-signet branch from 6ba0d07 to cc7d24f Compare

October 16, 2018 22:18

@jtimon jtimon added 0.14.1 and removed 0.14.1 labels

Oct 16, 2018

NOMERGE: [0.17] Signet implementation #433

Copy link

Contributor Author

jtimon commented Oct 16, 2018 •

edited

Loading

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

Closed

3 tasks

@instagibbs instagibbs added the needs port label

Mar 25, 2019

Copy link

Contributor

instagibbs commented Mar 28, 2019

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

edited

Loading

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