[0.17] blockheaders include block height, nodes validate it #431

instagibbs · 2018-10-12T18:48:58Z

The feature is enabled by default in custom chains; cannot be activated in legacy chains.

jtimon

Also, could use a rebase and move the -con_blockheightinheader to chainparams.

src/chain.h

src/chainparams.cpp

jtimon · 2018-10-16T19:52:39Z

src/primitives/block.h

@@ -41,6 +45,9 @@ class CBlockHeader
        READWRITE(hashPrevBlock);
        READWRITE(hashMerkleRoot);
        READWRITE(nTime);
+        if (gArgs.GetBoolArg("-con_blockheightinheader", true)) {


I think you need g_block_height_in_header here too

instagibbs · 2018-10-16T20:08:53Z

forgot to push my fixes that I believe fixed things. Will switch to chainparams setup next.

instagibbs · 2018-10-18T19:21:49Z

Feature complete now, just no tests.

jtimon · 2018-10-19T16:35:20Z

src/chainparams.cpp

@@ -118,7 +118,11 @@ class CMainParams : public CChainParams {

        genesis = CreateGenesisBlock(1231006505, 2083236893, 0x1d00ffff, 1, 50 * COIN);
        consensus.hashGenesisBlock = genesis.GetHash();
-        assert(consensus.hashGenesisBlock == uint256S("0x000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f"));
+        consensus.blockheight_in_header = gArgs.GetBoolArg("-con_blockheightinheader", false);


No, just don't use -con_blockheightinheader at all outside of CCustomParams and you shouldn't need to touch neither CMainParams, CTestNetParams or CRegTestParams, specially not their asserts.
Perhaps it would be interesting to rebase on top of #436 and add a test like this:

{ 'memo': 'default_style with height in header', 'genesis': 'c03f16ae9e2980de2b61fd6dc84af8ac4a37bea928af632166a6b36c5c871ddd', 'args': [ '-con_genesis_style=default_style', '-con_blockheightinheader', ], },

It creates a new style genesis block for each network when iterating through network types during load, then hits the assert.

That means blockheight_in_header is not really acting as false for main, test and regtest, no?

It is, if you don't set -con_blockheightinheader to true

instagibbs · 2018-10-22T14:38:12Z

I'm getting build failures for windows and other builds due to gArgs in block.h.

instagibbs · 2018-10-22T22:58:23Z

tricked PR to refresh by closing/opening

instagibbs · 2018-10-25T17:51:29Z

Took essential fixes from #442 and squashed.

instagibbs · 2018-10-26T17:06:59Z

Rebased, fixed some tests.

instagibbs · 2018-10-29T19:29:59Z

Finally resolves all issues and tests. Ready for review.

mword · 2018-10-31T18:04:06Z

Note that for me many tests fail with test/functional/test_runner.py --extended . The extended functional tests pass on the elements-0.17 branch.

instagibbs · 2018-10-31T18:31:51Z

@mword ah good catch. Will take a look.

instagibbs · 2018-11-01T18:33:01Z

Looks like upstream moved all but two tests into the normal testing regime. We should probably mirror that if possible.

instagibbs · 2018-11-05T15:47:27Z

rebased onto tip to catch most extended failures

instagibbs · 2018-11-05T20:57:13Z

All builds passing, squashed.

mword · 2018-11-06T01:06:21Z

tACK with one caveat: I ran test_runner.py twice once with --extended and then once without, the second run (without --extended) the rpc_rawtransaction.py test failed. I could not reproduce the failure. I ran test_runner.py again several with no problems and I ran the rpc_rawtransaction.py test by itself a number of times with no failures.

stevenroose · 2018-11-06T18:55:36Z

tACK a8c6fcd

instagibbs · 2018-11-13T15:15:59Z

will cherry-pick Blockstream/liquid#3 when merged

stevenroose · 2018-11-15T16:22:55Z

ACK when you merge liquid#3 :)

instagibbs · 2018-11-15T18:01:03Z

@stevenroose oh it appears I already mirror this logic in liquid#3, great minds yada yada

instagibbs · 2018-11-19T22:43:54Z

It's never (de)serialed as non zero if not active. I can add the logic of it makes you uncomfortable.

…

On Mon, Nov 19, 2018, 5:32 PM Luke Dashjr ***@***.*** wrote: ***@***.**** commented on this pull request. ------------------------------ In src/chain.h <#431 (comment)> : > @@ -403,6 +411,7 @@ class CDiskBlockIndex : public CBlockIndex READWRITE(hashPrev); READWRITE(hashMerkleRoot); READWRITE(nTime); + READWRITE(block_height); I don't understand why this doesn't matter. Bitcoin block indexes sure won't have height here... — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#431 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgC07ez5yBT_9TmSEVIhSFxuoStdBKAks5uwzF9gaJpZM4XaAR-> .

luke-jr · 2018-11-19T22:33:00Z

src/chain.h

@@ -403,6 +407,7 @@ class CDiskBlockIndex : public CBlockIndex
        READWRITE(hashPrev);
        READWRITE(hashMerkleRoot);
        READWRITE(nTime);
+        READWRITE(block_height);


I don't understand why this doesn't need to be conditional. Bitcoin block indexes sure won't have height here...

I'm not sure myself, but Greg discussed it with Jorge here: #431 (comment)

luke-jr · 2018-11-19T22:33:42Z

src/chainparams.cpp

@@ -432,6 +432,10 @@ class CCustomParams : public CRegTestParams {
        // No subsidy for custom chains by default
        consensus.genesis_subsidy = args.GetArg("-con_blocksubsidy", 0);

+        // Note: This global is needed to avoid circular dependency


with chainparams (ie if the global was a field in chainparams), because chainparams depends on block.

luke-jr · 2018-11-19T22:35:32Z

src/miner.cpp

@@ -168,6 +168,9 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc
    // Fill in header
    pblock->hashPrevBlock  = pindexPrev->GetBlockHash();
    UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev);
+    if (g_con_blockheightinheader) {
+        pblock->block_height = nHeight;


Assignment probably doesn't need to be conditional.

luke-jr · 2018-11-19T22:43:35Z

test/functional/test_framework/blocktools.py

@@ -44,14 +44,15 @@
 # From BIP141
 WITNESS_COMMITMENT_HEADER = b"\xaa\x21\xa9\xed"

-def create_block(hashprev, coinbase, ntime=None):
+def create_block(hashprev, coinbase, block_height, ntime=None):


Modifying the function signature like this is begging for silent bugs.

Instead, add block_height to the end, and always specify it by name (enforce with PEP 3102; ie, (hashprev, coinbase, ntime=None, *, block_height)).

Ah that's what the random asterixes are for! :) Thanks

Another option is to make ntime non optional.

luke-jr · 2018-11-19T22:45:00Z

test/functional/test_framework/blocktools.py

@@ -44,14 +44,15 @@
 # From BIP141
 WITNESS_COMMITMENT_HEADER = b"\xaa\x21\xa9\xed"

-def create_block(hashprev, coinbase, ntime=None):
+def create_block(hashprev, coinbase, block_height, ntime=None):


Now that block_height is provided here, suggest making coinbase optional since almost all uses just call create_coinbase(block_height). Probably doesn't matter much unless/until this goes upstream...

I'd have to implement a CScriptNum encoder in python.... sigh I'll do it :)

Huh? create_coinbase (and serialize_script_num) already exists..

de-serialize*. I want to grab it directly from the coinbase.

luke-jr · 2018-11-19T23:08:11Z

It's never (de)serialed as non zero if not active.

Why wouldn't it take an existing CBlockIndex serialisation, and turn that bits into height, and nonce into bits?

jtimon · 2018-11-22T18:51:19Z

src/chainparamsbase.cpp

@@ -26,6 +26,7 @@ void SetupChainParamsBaseOptions()
    gArgs.AddArg("-seednode=<ip>", "Use specified node as seed node. This option can be specified multiple times to connect to multiple nodes. (custom only)", true, OptionsCategory::CHAINPARAMS);
    gArgs.AddArg("-con_blocksubsidy", "Defines the amount of block subsidy to start with, at genesis block.", false, OptionsCategory::CHAINPARAMS);
    gArgs.AddArg("-con_connect_coinbase", "Connect outputs in genesis block to utxo database.", false, OptionsCategory::CHAINPARAMS);
+    gArgs.AddArg("-con_blockheightinheader", "Whether the chain includes the block height directly in the header, for easier validation of block height in low-resource environments.", false, OptionsCategory::CHAINPARAMS);


Perhaps indicate that defaults to true here?

jtimon · 2018-11-22T19:09:37Z

I dislike that this modifies many unrelated tests (because the new option defaults to true) instead of just copying or creating a specific one to test this. That's bad for rebases.
Is it really necessary to default to true?

instagibbs · 2018-11-22T19:13:06Z

The testing it provides is valuable (caught a number of things I hadn't considered) but yes more effort could be made to not have so many lines change. I need a python implementation of CScruptNum encoding IIRC to not require a bunch of additional arguments everywhere.

…

On Thu, Nov 22, 2018, 2:09 PM Jorge Timón ***@***.*** wrote: I dislike that this modifies many unrelated tests (because the new option defaults to true) instead of just copying or creating a specific one to test this. That's bad for rebases. Is it really necessary to default to true? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#431 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgC01yByyEepugV8U7BZygUdZZvmPeqks5uxvZzgaJpZM4XaAR-> .

jtimon · 2018-11-22T19:14:49Z

Or you can just change the default to false, no? Perhaps duplicate and modify some tests if that has value, but run all of them with this set to true.
What am I missing?

instagibbs · 2018-11-22T19:19:11Z

We duplicated the tests do we're already testing with it off. Testing with it on as well allows better testing coverage when possible. I can take a look at the tests and see if we're testing anything meaningful. If not I can break out a new test I guess that specifically tests what we care about.

…

On Thu, Nov 22, 2018, 2:14 PM Jorge Timón ***@***.*** wrote: Or you can just change the default to false, no? Perhaps duplicate and modify some tests if that has value, but run all of them with this set to true. What am I missing? — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#431 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgC006W5VysegSHwEND83UCtS64vkKeks5uxveqgaJpZM4XaAR-> .

instagibbs · 2018-11-26T17:08:39Z

Pushed an update that is able to extract the block height directly from the coinbase input, and places it in the header. This shrinks the test diff by quite a bit.

instagibbs · 2018-11-26T21:45:59Z

squashed, passing tests, and ready for final review

The block height validation is gated by the startup option `con_blockheightinheader`, accesible in custom chains only using option `chain=<name>`. Only if set to true will this field be (de)serialized and evaluated in consensus logic. Otherwise the field is simply ignored, other than being stored in memory as the default value 0.

instagibbs · 2018-11-27T14:52:25Z

rebased and added default value message in help of true(assuming custom chains since that will be default later)

stevenroose · 2018-11-27T16:08:50Z

cool, tACK 2bfabb3!

2bfabb3 blockheaders include block height, nodes validate it (Gregory Sanders) 8f6bedf functional framework support for CScriptNum decode (Gregory Sanders) Pull request description: The feature is enabled by default in custom chains; cannot be activated in legacy chains. Tree-SHA512: d033b7dd0d853e5693efda584494cf58ed7ff43ce32d1f5d44e4ee52907b989e434c8dbb28a0f843da8b799548924d2499e8759a96ed5a7191b95f22b29c7f6e

…n third-party link action 2ccde2f qt: hyphenate usage of third-party modifier (Jarol Rodriguez) 8177578 qt: ensure seperator when adding third-party transaction links (Jarol Rodriguez) a70a980 qt: improve text for open third-party tx url action (Jarol Rodriguez) 9980f4a qt, refactor: simplify third-party tx url action through overload (Jarol Rodriguez) Pull request description: [#4092](bitcoin/bitcoin#4092) introduced the ability to open up a transaction in a block explorer. This improves the related code by simplifying the addition and connection of the action through an [overloaded](https://doc.qt.io/archives/qt-5.9/qmenu.html#addAction-5) `addAction` function and prepends action description text to the host, "Show in". The reason to add this text is to make it clear what the action does. It also creates a clearer mental correlation between a user doing the work to add the 3rd-party tx link and this new menu action popping up. This updates the setting text so that "third-party" is hyphenated. It should be hyphenated because it is being used as a modifier of both "URL" and "transaction URLs". Additionally, this fixes ElementsProject#431 by ensuring that the seperator will be added before creating action. Screenshots of visual changes: **Context menu actions** | master | pr | |--------------|--------| | <img width="248" alt="3pt-master" src="https://user-images.githubusercontent.com/23396902/134618354-00278ac6-5094-44ee-8ba7-fe648fdcb7d2.png"> | <img width="248" alt="3pt-pr" src="https://user-images.githubusercontent.com/23396902/134618364-ddb64269-e5ee-40af-a2a6-1922001b6f4e.png"> | **Setting text** (tooltip text containing usage of "third-party" is also properly hyphenated) | master | pr | |--------------|--------| | ![unnamed](https://user-images.githubusercontent.com/23396902/134854070-fb299ba5-3491-487f-b37f-c0cd96514353.png) | ![pr-hyphenate](https://user-images.githubusercontent.com/23396902/134854127-88630cc2-a178-4376-a569-f413f66eba0d.png) | ACKs for top commit: stratospher: tested ACK 2ccde2f. promag: Code view ACK 2ccde2f. hebasto: ACK 2ccde2f Tree-SHA512: 8dfcd539a1d41c8abf3c8208d150d1480d4ef81a008de826299e8bad1dfa6e3c49dc76d041c5946fafcf0b033eebb9b9fbd3d49ba6d8af93dd388c488e92f143

instagibbs mentioned this pull request Oct 12, 2018

Update elements to Bitcoin Core version 0.17.0 #398

Closed

30 tasks

instagibbs force-pushed the height_in_header branch from 2a12b30 to a89d0eb Compare October 16, 2018 18:11

jtimon suggested changes Oct 16, 2018

View reviewed changes

instagibbs force-pushed the height_in_header branch from a89d0eb to 32c7386 Compare October 16, 2018 20:08

jtimon added the 0.17 label Oct 16, 2018

instagibbs force-pushed the height_in_header branch from 32c7386 to d8ee236 Compare October 18, 2018 19:16

instagibbs force-pushed the height_in_header branch 2 times, most recently from 04c8fe5 to 97b714f Compare October 18, 2018 20:59

jtimon reviewed Oct 19, 2018

View reviewed changes

instagibbs force-pushed the height_in_header branch from 97b714f to 3f3b3ab Compare October 22, 2018 14:19

jtimon mentioned this pull request Oct 22, 2018

[0.17] MODIFIED: blockheaders include block height, nodes validate it #442

Closed

instagibbs closed this Oct 22, 2018

instagibbs reopened this Oct 22, 2018

instagibbs force-pushed the height_in_header branch from ba230a7 to 529efb1 Compare October 25, 2018 17:44

instagibbs changed the title ~~[WIP, 0.17] blockheaders include block height, nodes validate it~~ [WIP] blockheaders include block height, nodes validate it Oct 25, 2018

instagibbs force-pushed the height_in_header branch from 529efb1 to 02bfec8 Compare October 25, 2018 17:47

instagibbs changed the title ~~[WIP] blockheaders include block height, nodes validate it~~ [0.17] blockheaders include block height, nodes validate it Oct 25, 2018

instagibbs force-pushed the height_in_header branch from 02bfec8 to 15e8f6e Compare October 26, 2018 17:06

instagibbs force-pushed the height_in_header branch 4 times, most recently from e4a3331 to 977ea1a Compare October 29, 2018 19:29

instagibbs force-pushed the height_in_header branch from 977ea1a to 29763da Compare November 5, 2018 15:47

instagibbs force-pushed the height_in_header branch from 51a9458 to a8c6fcd Compare November 5, 2018 20:57

luke-jr suggested changes Nov 19, 2018

View reviewed changes

jtimon reviewed Nov 22, 2018

View reviewed changes

instagibbs force-pushed the height_in_header branch from 5d0d66c to 5a5941a Compare November 26, 2018 21:45

instagibbs added 2 commits November 27, 2018 09:45

functional framework support for CScriptNum decode

8f6bedf

instagibbs force-pushed the height_in_header branch from 5a5941a to 2bfabb3 Compare November 27, 2018 14:51

instagibbs merged commit 2bfabb3 into ElementsProject:elements-0.17 Nov 27, 2018

[0.17] blockheaders include block height, nodes validate it #431

[0.17] blockheaders include block height, nodes validate it #431

Conversation

instagibbs commented Oct 12, 2018 • edited Loading

jtimon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Oct 16, 2018

instagibbs commented Oct 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

instagibbs commented Oct 22, 2018

instagibbs commented Oct 22, 2018

instagibbs commented Oct 25, 2018

instagibbs commented Oct 26, 2018

instagibbs commented Oct 29, 2018

mword commented Oct 31, 2018

instagibbs commented Oct 31, 2018

instagibbs commented Nov 1, 2018

instagibbs commented Nov 5, 2018

instagibbs commented Nov 5, 2018

mword commented Nov 6, 2018

stevenroose commented Nov 6, 2018

instagibbs commented Nov 13, 2018

stevenroose commented Nov 15, 2018

instagibbs commented Nov 15, 2018

instagibbs commented Nov 19, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luke-jr commented Nov 19, 2018

Choose a reason for hiding this comment

jtimon commented Nov 22, 2018

instagibbs commented Nov 22, 2018 via email

jtimon commented Nov 22, 2018

instagibbs commented Nov 22, 2018 via email

instagibbs commented Nov 26, 2018

instagibbs commented Nov 26, 2018

instagibbs commented Nov 27, 2018

stevenroose commented Nov 27, 2018

instagibbs commented Oct 12, 2018 •

edited

Loading