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

HF: Move block.proof.challenge to Consensus::Params::signblockScript #114

Conversation

jtimon
Copy link
Contributor

@jtimon jtimon commented Feb 2, 2017

Replaces #111
Since the scriptPubKey for signing blocks never changes, there's no point in repeating it with every block header. This saves more space in memory the more signatures there are in a particular chain.

Also includes a hash of fedpegscript and signblockscript in the genesis block which is also a hf, but makes the genesis block depend on them.

@instagibbs
Copy link
Collaborator

I think these changes should be proposed separately or in sequence. One changes the header format which can effect how the HSMs operate and will need more review from more people.

@jtimon
Copy link
Contributor Author

jtimon commented Mar 7, 2017

As discussed with @instagibbs , since there's no hurry to do the hardfork, the first change can wait for review on the second one.

@Christewart
Copy link
Contributor

utACK 38bf14a

@jtimon jtimon force-pushed the 0.13-elements-blocksign-hf2 branch from 38bf14a to 3bb2140 Compare April 11, 2017 03:50
@jtimon
Copy link
Contributor Author

jtimon commented Apr 11, 2017

Needed rebase

@jtimon
Copy link
Contributor Author

jtimon commented Apr 11, 2017

Sorry, some unittests are failing

TEST                           | PASSED | DURATION

mempool_resurrect_test.py      | True   | 3 s
p2p-fullblocktest.py           | True   | 3 s
txn_doublespend.py --mineblock | True   | 5 s
txn_clone.py                   | True   | 6 s
rest.py                        | True   | 6 s
mempool_spendcoinbase.py       | True   | 4 s
mempool_reorg.py               | True   | 4 s
httpbasics.py                  | True   | 4 s
multi_rpc.py                   | True   | 4 s
wallet.py                      | True   | 12 s
proxy_test.py                  | True   | 6 s
merkle_blocks.py               | True   | 6 s
signrawtransactions.py         | True   | 3 s
fundrawtransaction.py          | True   | 5 s
getchaintips.py                | True   | 16 s
decodescript.py                | True   | 3 s
sendheaders.py                 | True   | 4 s
prioritise_transaction.py      | True   | 3 s
invalidblockrequest.py         | True   | 3 s
invalidtxrequest.py            | True   | 3 s
zapwallettxes.py               | True   | 16 s
keypool.py                     | True   | 7 s
abandonconflict.py             | True   | 4 s
segwit.py                      | True   | 4 s
signmessages.py                | True   | 3 s
reindex.py                     | True   | 14 s
nodehandling.py                | True   | 15 s
zmq_test.py                    | True   | 5 s
listtransactions.py            | False  | 68 s
receivedby.py                  | False  | 71 s
rawtransactions.py             | False  | 77 s
confidential_transactions.py   | False  | 84 s

ALL                            | False  | 471 s (accumulated)

Runtime: 104 s

instagibbs and others added 2 commits April 19, 2017 23:43
Since the scriptPubKey for signing blocks never changes, there's no
point in repeating it with every block header.
@jtimon jtimon force-pushed the 0.13-elements-blocksign-hf2 branch from 3bb2140 to 3d83e51 Compare April 19, 2017 21:49
@jtimon
Copy link
Contributor Author

jtimon commented Apr 19, 2017

Updated with some feedback from @jonasnick

@instagibbs
Copy link
Collaborator

instagibbs commented Apr 19, 2017 via email

@jtimon
Copy link
Contributor Author

jtimon commented Apr 19, 2017

Yes, sorry.
Basically we discussed the fields "bits" and "target" aren't needed in the existing places neither in rpc/blockchain nor in getBlockTemplate and new fields for the signblock script as a hex string and as assembly (as was done in the previous places) in getblockchaininfo can replace them.
The hex is necessary for a block signer. Previously, it could simply extract the the challenge from the serialized block header.
And those are the changes I made to the last commit, the first commit wasn't modified, but I rebased.

@jonasnick
Copy link
Contributor

jonasnick commented Apr 19, 2017

EDIT: jtimon already answered

I wanted to be able to get the signblockscript as hex somehow from the RPC which is why @jtimon added signblockhex to the getblockchaininfo RPC. Without the change you can only get the signblockscript in asm string format with getblock.

Using Bitcoin genesis mainnet hash for pegged-bitcoin asset id was a
mistake in the first place.
@jtimon
Copy link
Contributor Author

jtimon commented Apr 20, 2017

Sorry, added one commit, potentially to squash to the previous one, or leave for #164

Copy link
Collaborator

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

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

utACK a71aa8b

}

bool CheckChallenge(const CBlockHeader& block, const CBlockIndex& indexLast, const Consensus::Params& params)
{
return block.proof.challenge == indexLast.proof.challenge;
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What use is this stub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I thought we may want to change the challenges, for example, for key rotation. This function would allow to restrict the allowed transitions from one challenge to another. Since we're not using any of that, there's no point in maintaining the challenge field in the header, but we can leave the functions here in case we want to restore the field to do something like that later.

@jtimon
Copy link
Contributor Author

jtimon commented Jun 22, 2017

Replaced by #189

@jtimon jtimon closed this Jun 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants