HF22 (HFNET) Consensus Changes by DeckerSU · Pull Request #538 · GLEECBTC/komodo-daemon

DeckerSU · 2022-05-28T15:05:39Z

Don't merge this PR till:

S6 pubkeys will be 100% collected and corresponding PR with changes in komodo_globals.h, komodo_hardfork.h will be done.
S6 NN ops should participate in mandatory HFNET testing in which we should test everything with 50% of existing nodes at least, to estimate maximum gap between blocks. Probably HFNET should be re-created based on https://github.com/KomodoPlatform repo sources once again.
As originally this PR was in KomodoOcean repo, we should compare all changes once again line-by-line, to be sure that nothing is forgotten and nothing is mixed in logic. Of course, i did all checks by myself twice, but better to have few more pair of eyes open on this.
When nS6TestTimestamp and nS6TestHardforkHeight will be set in stone, we should set consensus.nHF22Height, as a week after nS6TestHardforkHeight to not overlap two important consensus changes. This should be done before merging this PR.
Patch for KomodoPlatform/komodo repo dev branch to match current HFNET spinned-up by @Alrighttt , i mean this, available here - hfnet-alrighttt-changes.zip.
Changing p2p port to 8880 also should be reverted, before merging this PR (!), this was done only for HFNET and testing purposes.

Currently i selected @ca333 , @Alrighttt and @smk762 for review, but later, after all tests, probably we should give a chance to look on all of these to @jmjatlanta and @dimxy .

should be reverted after all tests succeded on hfnet # Conflicts: # src/init.cpp # src/komodo_utils.cpp

height after which notary nodes allowed to mine extra mindiff block in case of stale, turned on by default on mainnet, and disabled at present for testnet and regtest.

hf22 rule will apply according to nAllowNotariesMineExtraBlockAfterStaleHeight set or not for current network, so no need to use hardcoded string check.

# Conflicts: # src/miner.cpp

next call to CheckProofOfWork will generate non-expected prioirty list, i.e. nth call: 1 - 0 1 2 3 4 5 ... 63 2 - 64 65 66 57 ... 127 3 - 128 129 130 ... 191 so, init of id should be non-static and outside lambda.

as HF22 changes will include few other consensus changes, better to give it more common name, not only extra block related for notaries.

also possible optimize of loop iterating txes, in case of height == 0, don't need to call komodo_block2height for each tx, call it once before iterating loop is enough. # Conflicts: # src/main.cpp

instead of block.nTime (pblock->nTime). This should prevent existance of txes, which are already added in mempool, but never will be added to a block (skipped on create blocktemplate stage), bcz of MTP too far away from block creation time in case of big gaps between blocks. # Conflicts: # src/miner.cpp

… error Before debug.log in case of komodo_validate_interest failed was like: 2022-04-29 03:14:33 AcceptToMemoryPool komodo_validate_interest failure 2022-04-29 03:14:33 ERROR: AcceptToMemoryPool: komodo_validate_interest failed We removed 1st message and added txid to 2nd. # Conflicts: # src/main.cpp

We don't need to call pindexPrev->GetMedianTimePast() on every mempool tx iteration, it's calculated before in nMedianTimePast. Also we don't need params variable, as we already have consensusParams defined above. # Conflicts: # src/miner.cpp

- re-format, remove useless comments and dead code. - remove unused variables. - remove set<CBitcoinAddress> setAddress and conditions containing it, bcz setAddress.size() is always 0, so these paths in the code are dead. - get rid of most pointer operations, instead of memcmp passed notarypub33, we construct targetP2PKScript and compare it with scripts from available utxos. - don't allow to spend unconfirmed 10k sat. utxos, bcz we don't ensure that they would be included in the same block. # Conflicts: # src/wallet/rpcwallet.cpp

to pass two params in a function earlier we used a (void **) to handle pointers on these 2 params in "array". also there was "hidden" parameter, this pointer itself, if pointer == 0 -> it before HF time, if pointer !=0 -> after HF time, and we should extract two params from array of passed pointers. of course it can be replaced with tuple, or boost::optional with tuple "inside", but much more simple way is to pass params (CScript and nLockTime) in komodo_notaryvin same way as they exits and assume that if passed CScript().size() == 0, it before HF and if size() > 0 - it after HF. # Conflicts: # src/miner.cpp

"Backport" of bitcoin/bitcoin#10534 to our codebase. TODO: consider to use latest prevector implementation from Bitcoin upstream.

TheComputerGenie · 2022-05-28T20:02:34Z

There seems to be a certain level of counter-productivity in doing this and not removing the random delay when there are "too few" external miners as well

DeckerSU · 2022-05-28T20:12:49Z

There seems to be a certain level of counter-productivity in doing this and not removing the random delay when there are "too few" external miners as well

This PR have no significant relationship to internal miner behavior. All substantial changes in internal miner that could be possible done in it will be done later, if will. What about delay you are talking about, yes, we are know about it, and it will be possible reconsidered / refactored / reduced / or, even removed, based on further analysis of the chain behavior / block creation, etc.

Main goal of this PR is to introduce new consensus rules, which will be able to help overcome "chain stuck" issue and "disappearing transactions" issue, in-case of big gaps between blocks.

TheComputerGenie · 2022-05-28T20:39:50Z

Main goal of this PR is to introduce new consensus rules, which will be able to help overcome "chain stuck" issue and "disappearing transactions" issue, in-case of big gaps between blocks.

Perhaps I used the wrong wording? If you want the chain to stick less and want NNs to assist in said goal then one important step in doing so would be removing a code block that intentionally delays them from sending blocks during the time you want them to be creating more blocks.

The flaw in the notion that "no significant relationship to internal miner behavior" can solve a problem that the "internal miner behavior" is coded to create is that it's coded to fight against the goal of the PR.

On one hand you're allowing NNs to mine more often, on the other hand you're keeping in code that, in most circumstances, disallows them from creating a block in the time frame you want them to create it in.

Edit: case in point,, literally right now...

DeckerSU · 2022-05-28T20:50:52Z

Perhaps I used the wrong wording? If you want the chain to stick less and want NNs to assist in said goal then one important step in doing so would be removing a code block that intentionally delays them from sending blocks during the time you want them to be creating more blocks.

Forget about internal miner implementation on a second. Let's imagine that part of NNs using it, and other part using other solution for creating blocks, you could imagine that NNs could create a block at the click of a finger 🪄, for example. In case of absent of "external" PoW miners (gpucount == 0) - removing the delay you pointed earlier will not help anyhow, in case of chain stuck. Bcz last 64 blocks were created by NNs, and we will face with a "no one allowed to mine a block" case. One of the intents of this PR is to rule out such cases.

And yes, we will return to timeframes, delays, and other "artificial things" in internal miner later, i.e. in other PRs.

p.s. Try to abstract of internal miner usage for now, i mean when thinking of "how it will be".

TheComputerGenie · 2022-05-28T21:06:44Z

p.s. Try to abstract of internal miner usage for now, i mean when thinking of "how it will be".

I would and I could, except for the fact that my mind is incapable of quietly seeing a thing fight against itself.
Both things should be done at the same time and with multiple testers.
Ignoring one part of the code while implementing a competing code tells the running node:

The network needs you to create an extra block now because the chain isn't moving due to lack of hashrate and we need blocks now! Oh, and also, part of the reason we need blocks now is that someone (maybe even you) possibly can't submit a block for another 14 minutes, even though they were allowed to create it 30 minutes ago and we needed it then because there haven't been enough hashrate blocks created.

I sometimes think that people reject reading my words just because they come from me.

DeckerSU · 2022-05-28T21:54:06Z

Both things should be done at the same time and with multiple testers. Ignoring one part of the code while implementing a competing code tells the running node:

It's not "ignoring". As i said above: "And yes, we will return to timeframes, delays, and other "artificial things" in internal miner later, i.e. in other PRs". At this stage let's separate internal miner things and consensus things. Within this PR we are talking only about consensus changes.

This reverts commit 2a54247.

DeckerSU · 2022-06-09T16:58:37Z

PR is updated. Time to review and merge together with #540.

Alrighttt

Consensus changes tested thoroughly via HFNET.

Thanks for the notaryvin clean up as well.

Alrighttt · 2022-05-28T16:39:21Z

src/chainparams.cpp

        vAlertPubKey = ParseHex("020e46e79a2a8d12b9b5d12c7a91adb4e454edfae43c0a0cb805427d2ac7613fd9");
        // (Zcash) vAlertPubKey = ParseHex("04b7ecf0baa90495ceb4e4090f6b2fd37eec1e9c85fac68a487f3ce11589692e4a317479316ee814e066638e1db54e37a10689b70286e6315b1087b6615d179264");
-        nDefaultPort = 7770;
+        nDefaultPort = 8880;


Suggested change

nDefaultPort = 8880;

nDefaultPort = 7770;

Do not change the p2p port in production.

Alrighttt · 2022-05-28T16:39:55Z

src/chainparamsseeds.h

-    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x68,0xee,0xdd,0x3d}, 7770},
-    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xd1,0xde,0x65,0xf7}, 7770},
-    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x67,0xc3,0x64,0x20}, 7770}
+    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xc7,0x7f,0x3c,0x8e}, 8880},


Suggested change

{{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xc7,0x7f,0x3c,0x8e}, 8880},

{{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xc7,0x7f,0x3c,0x8e}, 7770},

Alrighttt · 2022-05-28T16:40:05Z

src/chainparamsseeds.h

-    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xd1,0xde,0x65,0xf7}, 7770},
-    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x67,0xc3,0x64,0x20}, 7770}
+    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xc7,0x7f,0x3c,0x8e}, 8880},
+    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x68,0xee,0xdd,0x3d}, 8880},


Suggested change

{{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x68,0xee,0xdd,0x3d}, 8880},

{{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x68,0xee,0xdd,0x3d}, 7770},

Alrighttt · 2022-05-28T16:40:16Z

src/chainparamsseeds.h

-    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x67,0xc3,0x64,0x20}, 7770}
+    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xc7,0x7f,0x3c,0x8e}, 8880},
+    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x68,0xee,0xdd,0x3d}, 8880},
+    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xd1,0xde,0x65,0xf7}, 8880},


Suggested change

{{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xd1,0xde,0x65,0xf7}, 8880},

{{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xd1,0xde,0x65,0xf7}, 7770},

Alrighttt · 2022-05-28T16:40:25Z

src/chainparamsseeds.h

+    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xc7,0x7f,0x3c,0x8e}, 8880},
+    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x68,0xee,0xdd,0x3d}, 8880},
+    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xd1,0xde,0x65,0xf7}, 8880},
+    {{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x67,0xc3,0x64,0x20}, 8880}


Suggested change

{{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x67,0xc3,0x64,0x20}, 8880}

{{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0x67,0xc3,0x64,0x20}, 7770}

Alrighttt · 2022-05-28T16:43:28Z

src/komodo_port.c

 {
    if ( magic == 0x8de4eef9 )
-        return(7770);
+        return(8880);


Suggested change

return(8880);

return(7770);

Alrighttt · 2022-05-28T16:43:42Z

src/komodo_port.c

    {
        *magicp = 0x8de4eef9;
-        return(7770);
+        return(8880);


Suggested change

return(8880);

return(7770);

Alrighttt · 2022-05-28T16:43:53Z

src/komodo_utils.cpp

 {
    if ( magic == 0x8de4eef9 )
-        return(7770);
+        return(8880);


Suggested change

return(8880);

return(7770);

Alrighttt · 2022-05-28T16:44:04Z

src/komodo_utils.cpp

    {
        *magicp = 0x8de4eef9;
-        return(7770);
+        return(8880);


Suggested change

return(8880);

return(7770);

Alrighttt · 2022-05-28T16:44:15Z

src/komodo_utils.cpp

    {
        char fname[512],username[512],password[4096]; int32_t iter; FILE *fp;
-        ASSETCHAINS_P2PPORT = 7770;
+        ASSETCHAINS_P2PPORT = 8880;


Suggested change

ASSETCHAINS_P2PPORT = 8880;

ASSETCHAINS_P2PPORT = 7770;

Release v1.1.0

DeckerSU added 17 commits May 27, 2022 19:14

hfnet: change default p2p port to 8880

2a54247

should be reverted after all tests succeded on hfnet # Conflicts: # src/init.cpp # src/komodo_utils.cpp

introduce nAllowNotariesMineExtraBlockAfterStaleHeight consensus param

7c5019f

height after which notary nodes allowed to mine extra mindiff block in case of stale, turned on by default on mainnet, and disabled at present for testnet and regtest.

add hf22 validation code with debug logging

928d2e4

miner: remove unused roundrobin_delay and KOMODO_CHOSEN_ONE

7009be5

remove network id == "main" restriction from validation

5f86224

hf22 rule will apply according to nAllowNotariesMineExtraBlockAfterStaleHeight set or not for current network, so no need to use hardcoded string check.

miner: apply hf22 rule if applicable

2582f7e

# Conflicts: # src/miner.cpp

move nHF22NotariesPriorityRotateDelta to consensus params

23e3b68

avoid pitfall with static id in lambda

3cd68f6

next call to CheckProofOfWork will generate non-expected prioirty list, i.e. nth call: 1 - 0 1 2 3 4 5 ... 63 2 - 64 65 66 57 ... 127 3 - 128 129 130 ... 191 so, init of id should be non-static and outside lambda.

rename nAllowNotariesMineExtraBlockAfterStaleHeight to nHF22Height

73da83d

as HF22 changes will include few other consensus changes, better to give it more common name, not only extra block related for notaries.

move interest validation to from CheckBlock to ContextualCheckBlock

07bdb1d

also possible optimize of loop iterating txes, in case of height == 0, don't need to call komodo_block2height for each tx, call it once before iterating loop is enough. # Conflicts: # src/main.cpp

Clarify prevector::erase and avoid swap-to-clear

ff1bcbc

"Backport" of bitcoin/bitcoin#10534 to our codebase. TODO: consider to use latest prevector implementation from Bitcoin upstream.

fix formatting in debug message (extra/second block allowed case)

7d36ddd

DeckerSU requested review from Alrighttt, ca333 and smk762 May 28, 2022 15:05

ca333 added the under test/review label May 28, 2022

DeckerSU added 2 commits June 9, 2022 19:52

Revert "hfnet: change default p2p port to 8880"

4067d0a

This reverts commit 2a54247.

update nHF22Height as nS6HardforkHeight + ~1 week

3eb9d5b

Alrighttt previously approved these changes Jun 9, 2022

View reviewed changes

remove unnecessary comment

6930dcc

DeckerSU dismissed Alrighttt’s stale review via 6930dcc June 10, 2022 21:09

ca333 approved these changes Jun 11, 2022

View reviewed changes

ca333 merged commit 9606efc into GLEECBTC:dev Jun 11, 2022

who-biz pushed a commit to who-biz/komodo that referenced this pull request Jul 29, 2024

Merge pull request GLEECBTC#538 from Asherda/release-v1.1.0

801a5c9

Release v1.1.0

	{{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xc7,0x7f,0x3c,0x8e}, 8880},
	{{0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0x00,0xff,0xff,0xc7,0x7f,0x3c,0x8e}, 7770},

Conversation

DeckerSU commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheComputerGenie commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeckerSU commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheComputerGenie commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeckerSU commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheComputerGenie commented May 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeckerSU commented May 28, 2022

Uh oh!

DeckerSU commented Jun 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alrighttt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DeckerSU commented May 28, 2022 •

edited

Loading

TheComputerGenie commented May 28, 2022 •

edited

Loading

DeckerSU commented May 28, 2022 •

edited

Loading

TheComputerGenie commented May 28, 2022 •

edited

Loading

DeckerSU commented May 28, 2022 •

edited

Loading

TheComputerGenie commented May 28, 2022 •

edited

Loading

DeckerSU commented Jun 9, 2022 •

edited

Loading