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

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',
 ],
},

Copy link

Contributor Author

@instagibbs instagibbs Oct 22, 2018

Choose a reason for hiding this comment

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

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

Copy link

Contributor

@jtimon jtimon Oct 22, 2018

Choose a reason for hiding this comment

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

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

Copy link

Contributor Author

@instagibbs instagibbs Oct 22, 2018

Choose a reason for hiding this comment

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

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

@instagibbs instagibbs force-pushed the height_in_header branch from 97b714f to 3f3b3ab Compare

October 22, 2018 14:19

@instagibbs

Copy link

Contributor Author

instagibbs commented Oct 22, 2018

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

@jtimon jtimon mentioned this pull request

Oct 22, 2018

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

Closed

@instagibbs instagibbs closed this

Oct 22, 2018

@instagibbs instagibbs reopened this

Oct 22, 2018

@instagibbs

Copy link

Contributor Author

instagibbs commented Oct 22, 2018

tricked PR to refresh by closing/opening

@instagibbs instagibbs force-pushed the height_in_header branch from ba230a7 to 529efb1 Compare

October 25, 2018 17:44

@instagibbs 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 instagibbs force-pushed the height_in_header branch from 529efb1 to 02bfec8 Compare

October 25, 2018 17:47

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

Copy link

Contributor Author

instagibbs commented Oct 25, 2018

Took essential fixes from #442 and squashed.

@instagibbs instagibbs force-pushed the height_in_header branch from 02bfec8 to 15e8f6e Compare

October 26, 2018 17:06

@instagibbs

Copy link

Contributor Author

instagibbs commented Oct 26, 2018

Rebased, fixed some tests.

@instagibbs instagibbs force-pushed the height_in_header branch 4 times, most recently from e4a3331 to 977ea1a Compare

October 29, 2018 19:29

@instagibbs

Copy link

Contributor Author

instagibbs commented Oct 29, 2018

Finally resolves all issues and tests. Ready for review.

@mword

Copy link

mword commented Oct 31, 2018

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

Copy link

Contributor Author

instagibbs commented Oct 31, 2018

@mword ah good catch. Will take a look.

@instagibbs

Copy link

Contributor Author

instagibbs commented Nov 1, 2018

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

@instagibbs instagibbs force-pushed the height_in_header branch from 977ea1a to 29763da Compare

November 5, 2018 15:47

@instagibbs

Copy link

Contributor Author

instagibbs commented Nov 5, 2018

rebased onto tip to catch most extended failures

@instagibbs instagibbs force-pushed the height_in_header branch from 51a9458 to a8c6fcd Compare

November 5, 2018 20:57

@instagibbs

Copy link

Contributor Author

instagibbs commented Nov 5, 2018

All builds passing, squashed.

@mword

Copy link

mword commented Nov 6, 2018

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

Copy link

Contributor

stevenroose commented Nov 6, 2018

tACK a8c6fcd

@instagibbs

Copy link

Contributor Author

instagibbs commented Nov 13, 2018

will cherry-pick Blockstream/liquid#3 when merged

@stevenroose

Copy link

Contributor

stevenroose commented Nov 15, 2018

ACK when you merge liquid#3 :)

@instagibbs

Copy link

Contributor Author

instagibbs commented Nov 15, 2018

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

@instagibbs

Copy link

Contributor Author

instagibbs commented Nov 19, 2018 via email

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

luke-jr suggested changes

Nov 19, 2018

View reviewed changes

src/chain.h

READWRITE(hashPrev);

READWRITE(hashMerkleRoot);

READWRITE(nTime);

READWRITE(block_height);

Copy link

Contributor

@luke-jr luke-jr Nov 19, 2018

Choose a reason for hiding this comment

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

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

Copy link

Contributor

@stevenroose stevenroose Nov 22, 2018

Choose a reason for hiding this comment

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

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

src/chainparams.cpp

// No subsidy for custom chains by default

consensus.genesis_subsidy = args.GetArg("-con_blocksubsidy", 0);

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

Copy link

Contributor

@luke-jr luke-jr Nov 19, 2018

Choose a reason for hiding this comment

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

Copy link

Contributor

@jtimon jtimon Nov 22, 2018

Choose a reason for hiding this comment

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

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

src/miner.cpp

pblock->hashPrevBlock = pindexPrev->GetBlockHash();

UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev);

if (g_con_blockheightinheader) {

pblock->block_height = nHeight;

Copy link

Contributor

@luke-jr luke-jr Nov 19, 2018

Choose a reason for hiding this comment

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

Assignment probably doesn't need to be conditional.

test/functional/test_framework/blocktools.py Outdated

WITNESS_COMMITMENT_HEADER = b"\xaa\x21\xa9\xed"

def create_block(hashprev, coinbase, ntime=None):

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

Copy link

Contributor

@luke-jr luke-jr Nov 19, 2018

Choose a reason for hiding this comment

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

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

Copy link

Contributor

@stevenroose stevenroose Nov 22, 2018

Choose a reason for hiding this comment

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

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

Copy link

Contributor

@jtimon jtimon Nov 22, 2018

Choose a reason for hiding this comment

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

Another option is to make ntime non optional.

test/functional/test_framework/blocktools.py Outdated

WITNESS_COMMITMENT_HEADER = b"\xaa\x21\xa9\xed"

def create_block(hashprev, coinbase, ntime=None):

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

Copy link

Contributor

@luke-jr luke-jr Nov 19, 2018

Choose a reason for hiding this comment

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

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

Copy link

Contributor Author

@instagibbs instagibbs Nov 26, 2018

Choose a reason for hiding this comment

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

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

Copy link

Contributor

@luke-jr luke-jr Nov 26, 2018

Choose a reason for hiding this comment

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

Huh? create_coinbase (and serialize_script_num) already exists..

Copy link

Contributor Author

@instagibbs instagibbs Nov 26, 2018

Choose a reason for hiding this comment

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

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

@luke-jr

Copy link

Contributor

luke-jr commented Nov 19, 2018

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

jtimon reviewed

Nov 22, 2018

View reviewed changes

src/chainparamsbase.cpp Outdated

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

Copy link

Contributor

@jtimon jtimon Nov 22, 2018

Choose a reason for hiding this comment

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

Perhaps indicate that defaults to true here?

@jtimon

Copy link

Contributor

jtimon commented Nov 22, 2018

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

Copy link

Contributor Author

instagibbs commented Nov 22, 2018 via email

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

Copy link

Contributor

jtimon commented Nov 22, 2018

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

Copy link

Contributor Author

instagibbs commented Nov 22, 2018 via email

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

Copy link

Contributor Author

instagibbs commented Nov 26, 2018

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 instagibbs force-pushed the height_in_header branch from 5d0d66c to 5a5941a Compare

November 26, 2018 21:45

@instagibbs

Copy link

Contributor Author

instagibbs commented Nov 26, 2018

squashed, passing tests, and ready for final review

instagibbs added 2 commits

November 27, 2018 09:45

@instagibbs


 functional framework support for CScriptNum decode

8f6bedf

@instagibbs


 blockheaders include block height, nodes validate it

2bfabb3

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 instagibbs force-pushed the height_in_header branch from 5a5941a to 2bfabb3 Compare

November 27, 2018 14:51

@instagibbs

Copy link

Contributor Author

instagibbs commented Nov 27, 2018

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

@stevenroose

Copy link

Contributor

stevenroose commented Nov 27, 2018

cool, tACK 2bfabb3!

@instagibbs instagibbs merged commit 2bfabb3 into ElementsProject:elements-0.17

Nov 27, 2018

instagibbs added a commit that referenced this pull request

Nov 27, 2018

@instagibbs


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

8a472b9

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

delta1 pushed a commit to delta1/elements that referenced this pull request

Apr 28, 2023

@hebasto


 Merge bitcoin-core/gui#430 : Improvements to the open up transaction i...

ba1a82e

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

Labels

0.17

5 participants

@instagibbs @mword @stevenroose @luke-jr @jtimon

Conversation

@instagibbs instagibbs commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

@jtimon jtimon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

@jtimon jtimon Oct 16, 2018

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Oct 16, 2018

Uh oh!

instagibbs commented Oct 18, 2018

Uh oh!

@jtimon jtimon Oct 19, 2018

Choose a reason for hiding this comment

Uh oh!

@instagibbs instagibbs Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

@jtimon jtimon Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

@instagibbs instagibbs Oct 22, 2018

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Oct 22, 2018

Uh oh!

instagibbs commented Oct 22, 2018

Uh oh!

instagibbs commented Oct 25, 2018

Uh oh!

instagibbs commented Oct 26, 2018

Uh oh!

instagibbs commented Oct 29, 2018

Uh oh!

mword commented Oct 31, 2018

Uh oh!

instagibbs commented Oct 31, 2018

Uh oh!

instagibbs commented Nov 1, 2018

Uh oh!

instagibbs commented Nov 5, 2018

Uh oh!

instagibbs commented Nov 5, 2018

Uh oh!

mword commented Nov 6, 2018

Uh oh!

stevenroose commented Nov 6, 2018

Uh oh!

instagibbs commented Nov 13, 2018

Uh oh!

stevenroose commented Nov 15, 2018

Uh oh!

instagibbs commented Nov 15, 2018

Uh oh!

instagibbs commented Nov 19, 2018 via email

Uh oh!

@luke-jr luke-jr Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

@stevenroose stevenroose Nov 22, 2018

Choose a reason for hiding this comment

Uh oh!

@luke-jr luke-jr Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

@jtimon jtimon Nov 22, 2018

Choose a reason for hiding this comment

Uh oh!

@luke-jr luke-jr Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

@luke-jr luke-jr Nov 19, 2018

Choose a reason for hiding this comment

Uh oh!

@stevenroose stevenroose Nov 22, 2018

Choose a reason for hiding this comment

@instagibbs instagibbs commented Oct 12, 2018 •

edited

Loading