blockchain: allow optimistic block insertion in blockchain #3584

g11tech · 2024-08-13T18:35:02Z

re-imagining the previous work (#3548) of extending blockchain for skeleton use based on the feedback

g11tech · 2024-08-13T18:48:06Z

packages/blockchain/src/blockchain.ts

-        const { header } = block
-        const blockHash = header.hash()
-        const blockNumber = header.number
-        let td = header.difficulty


this diff has primarily shifted into the else part of the optimistic condition on L414 with some basic initializations still out side the if {} else {} block

… number to ahash and rebuild canonical while backfilling

…ctionality

holgerd77 · 2024-08-19T12:41:45Z

packages/blockchain/src/db/operation.ts

@@ -14,6 +14,8 @@ import {

 import type { CacheMap } from './manager.js'

+export type OptimisticOpts = { fcUed: boolean; linked?: boolean }
+


These options are not really "understandable" in a blockchain context respectively bring in concepts from the client (FcU), and a blockchain "should not need to know what an FcU is". 😛

Can we rename this more to the point, so with what this is doing here from the blockchain perspective (I honestly also cannot read this out of the name and bring the two things together)?

Also linked I also can't directly read what is meant here.

These options should - if we keep - also move to types.ts since they become part of the official API eventually.

(maybe then directly give this some code docs)

… optimistic without moving head but track completion by total difficulty index

holgerd77 · 2024-08-27T11:34:35Z

Don't have anywhere near the full picture, but "total difficulty" sounds dangerous given that post-merge this is not used any more? 🤔

…insertion test

holgerd77 · 2024-09-16T11:21:39Z

packages/blockchain/examples/4444.ts

+  const common = new Common({ chain: Mainnet, hardfork: Hardfork.Shanghai })
+  // Use the safe static constructor which awaits the init method
+  const blockchain = await createBlockchain({
+    validateBlocks: false, // Skipping validation so we can make a simple chain without having to provide complete blocks


Ok, so the validateBlocks flag now also allows to insert blocks not being in the canoncial order, is this correct? (or was this behavior already there bofore) I generally do like this! 🙂

So, and - otherway around - if validateBlocks: true, then it remains forbidden to not go the normal path, right (or not), so 1,2,3,4,...?

Or, not sure if I am fully like it: can't we just fully validate again after this one not validated block (15537393n + 500n). So isn't it only this one block which can't be validated (and therefore would be something like a checkpoint) and then we can (and should) turn validation on again? Or can we do this with the current API already in this example?

i just picked the flag from other example, but i think yes we can enable the flag, and yes if the parent is missing then it won't be validated, so optimistic inserts will happen

but when you have put an anchor and move the head forward (which again happens through the forward putblocks along the backfilled chain), then they should be validated

i will remove this from the example and see if it works (most probably should)

Ah, forget about this, this is just simplification for the example, right?

g11tech · 2024-09-16T11:33:54Z

packages/blockchain/src/blockchain.ts

-        if (this._validateConsensus) {
-          await this.consensus!.validateConsensus(block)
-        }
+        // check if head is still canonical i.e. if this is a block insertion on tail or reinsertion


fyi this function is in need to cleanup so ignore for now

holgerd77 · 2024-09-16T11:43:28Z

packages/blockchain/src/db/operation.ts

@@ -13,6 +13,8 @@ import {

 import type { CacheMap } from './manager.js'

+export type PutOpts = { canonical?: boolean; parentTd?: bigint }


Can you move this over to types.ts and give both option some docs?
(also for (my) understanding)

This is still one of the crucial tasks so that we can work on some better understanding and documentation for these options and eventually come to better names.

So, if we have the definitions as comments over the options, this can be reviewed and alternatives/improvements can be suggested.

jochem-brouwer

Hi Gajinder, I gave this a look and had a few questions/comments 😄 👍

In general, for both documentation and for review purposes, could you document what this PR is trying to do, and what these new flags are? By the filenames I think this is in the context of EIP-4444. Could you document the new flags in the blockchain, as well as what an Optimistic / Complete block is here in this context? I understand that you are moving the skeleton part away from client and into blockchain, I think that makes sense. However I'm trying to think of how this changes the current behavior of blockchain with these new flags. For instance, what if I insert two optimistic blocks with the same number? What would getBlock(number) then return? In client by backfill we would then create two different subchains - right? How would that work here? 🤔

jochem-brouwer · 2024-09-16T15:30:39Z

packages/blockchain/examples/4444.ts

+      },
+    },
+    { common, setHardfork: true },
+  )


Nit: could these be packed in a for loop, and the "magic numbers" (500) be put as a constant?

jochem-brouwer · 2024-09-16T15:38:41Z

packages/blockchain/examples/optimistic.ts

+  await blockchain.putBlock(block3, { canonical: true })
+  headBlock = await blockchain.getCanonicalHeadBlock()
+  blockByHash = await blockchain.getBlock(block3.hash()).catch((e) => null)
+  blockByNumber = await blockchain.getBlock(block3.header.number).catch((e) => null)


We expect that this does not throw - right? So the catch should be removed? (same for blockByHash)

yes, its just a pattern used to log the status, sometimes blockByNumber will not be found depending upon the scenario

jochem-brouwer · 2024-09-16T15:41:07Z

packages/blockchain/examples/optimistic.ts

+  // 1. We can put any post merge block as 4444 anchor by using TTD as parentTD
+  // 2. For pre-merge blocks its prudent to supply correct parentTD so as to respect the
+  //    hardfork configuration as well as to determine the canonicality of the chain on future putBlocks
+  await blockchain.putBlock(block1, { parentTd: chainTTD })


If I were to validateBlocks = true and valdidateConsensus = true, what should happen if I put canonical = true here?

blockvalidation depends on parent availability in the blockchain and not on any other factor, for optimistic blocks it doesn't happen.

so lets say this was an optimistic block i.e. its parent was not in blockchain, validate blocks will be skipped. if you add canonical=true here, it will add the number => hash index and will run the head update rules (depending on if this is pow block or pos block) since parentTd has been specified and it will assume that it parent chain is confirmed and doesn't need to be added to blockchain

blockvalidation depends on parent availability in the blockchain

This is too implicit behavior. If we have set validateBlocks = true, blocks should be validated. Period. There shouldn't be inner auto-triggered exceptions from this rule (except maybe for the very explicit task of setting a checkpoint), otherwise one could not rely on the blockchain functionality anymore.

If one wants to deactivate block validation, we should add an option blockchain.setBlockValidation(false) or similar.

Guess this should then be accompanied by a flag for blockchain (if we go this route) to indicate that we might deal with a not-fully validated blockchain.

At least all these things (the "state" of the blockchain) should be transparent and requestable, and not just be "implicitly there".

jochem-brouwer · 2024-09-16T15:52:19Z

packages/blockchain/src/blockchain.ts

+        //    if they are optimistic, i.e. can't apply the normal rule
+        // 3. if canonical is not defined, then apply normal rules
+        if (opts?.canonical === false || (opts?.canonical === undefined && !isComplete)) {
+          if (parentTd !== undefined) {


Not sure about the behavior here. If opts.canonical is not set (undefined) then also isComplete is false, so this means that parentTd is undefined. So for PoW jobs blocks would jump to else clause here?

in this case: (opts?.canonical === undefined && !isComplete) would evaluate to true and the block will just be added to the blockchain as optimistic block, and no head update rules will be run or number => hash index modified

jochem-brouwer · 2024-09-16T15:53:13Z

packages/blockchain/src/blockchain.ts

+            if (
+              !block.isGenesis() &&
+              block.common.consensusType() !== ConsensusType.ProofOfStake &&
+              opts?.canonical === undefined


This will not work if opts.canonical is set to true, even if we don't have the parentTd, is this intended?

if parentTD is undefined, and this is not an explicity specified canonical PoS block, throw error because we can't determine the canonicality of this block

ideally such a scenario will not happen becuase we will not end up in else part of condition of L424, i will do a cleanup and see if we can come to a simplified logic, it could be redundant

jochem-brouwer · 2024-09-16T15:57:54Z

packages/blockchain/test/4444Optimistic.spec.ts

+      await blockchain.putBlock(block).catch((_e) => {
+        undefined
+      })
+      dbBlock = await blockchain.getBlock(block.hash()).catch((_e) => undefined)


I'm not entirely sure if I like this test setup. We expect dbBlock is undefined, but this can happen in 2 cases: (1) getBlock returns undefined and (2) getBlock throws. I think we should explicitly try/catch this and check for the expected behavior (either the error, or that it returns undefined).

true but since the condition below checks for it to be not undefined, the reason doesnt matter for the purposes of the test and will need to be debugged

jochem-brouwer · 2024-09-16T15:58:55Z

packages/blockchain/test/4444Optimistic.spec.ts

+      )
+
+      const td = await blockchain.getTotalDifficulty(block.hash()).catch((_e) => undefined)
+      assert.equal(td, undefined, 'block should be not be marked complete')


What is a "complete" block?

a complete block is the one which is confirmed to be linked all the way to the genesis via chain of parent blocks.

in our blockchain db, this is marked via block having the totalDifficulty written onto the chain. a boolean flag could also be used, but total difficulty serves well for both PoW and PoS blocks since PoW total difficulty is actually need to make head and canonicality decisions, while for PoS it just acts an an indicator.

while putting a block, if block's parent is in the blockchain and the parent also has totalDifficulty, then the new block is considered complete and it can move head

We should really really define (write down!) very clearly what we mean with both canonical and complete here. Again, maybe we are also "not there" with these names yet (e.g. complete for a block with this kind of definition is very misleading, one is just thinking in the realm of that the block has all txs, fields, .... We need to urgently find some better and more precise terminology here).

holgerd77 · 2024-09-18T08:20:42Z

packages/blockchain/examples/4444.ts

+  // 1. We can put any post merge block as 4444 anchor by using TTD as parentTD
+  // 2. For pre-merge blocks its prudent to supply correct parentTD so as to respect the
+  //    hardfork configuration as well as to determine the canonicality of the chain on future putBlocks
+  await blockchain.putBlock(block1, { parentTd: chainTTD })


I still do not fully understand this anchor concept. What is anchored and how?

block1 is the 4444 anchor i.e. the parent chain to block1 doesn't need to be added to the blockchain and blockchain can be build forward from here

it acts as a re-genesis anchor, one specifies an anchor by providing parentTD, for a PoS block parentTD can just be chainTTD, for a PoW block an actual parentTD should be added so as to eventually and correctly matchup with the terminal block TTD when forward chain blocks will be added on top of it

These are one of the things which are too implicit, at least on the API, maybe the DB level. I am still not fully grasping if the parentTd is also needed (for calculating/validating something) or is only set in some way to indicate the anchor status.

No matter how it is: this is unnecessarily implicit and we should think about the "anchor" (or call it checkpoint?) status and give this its own API and database representation.

I think "Checkpoint" might actually be the more common and established terminolgy.

So I think we definitely want to have some external relation in the DB where we store all the checkpoints (and not only have this in some very implicit form), and can e.g. extend our API with things like:

blockchain.getCheckpoints() blockchain.isCheckpoint(5n)

(or first there would be the question if there can be only one of these, but guess there can be several respectively would make sense?)

(wonder if we can even lign in the genesis block in this concept, guess this is exactly the same as a checkpoint?)

And then for setting these checkpoints, this should be very explicitly named. So if parentTd is still necessary, even if we store a Checkpoints -> [ Block Numbers ] relation, then this should minimally be provided with naming the option setCheckpoint: 560934n (and so provide the parentTD still, but set the very emphasis on this checkpoint setting, so one can read from the API call what is happening here).

acolytec3 · 2024-09-18T17:19:52Z

packages/blockchain/src/blockchain.ts

-        if (!block.isGenesis()) {
-          td += parentTd
-        }
+        // 1. if canonical is explicit false then just dump the block


What does canonical === false mean here? Just that we got a block that we know isn't canonical but we want to hold on to it "for future reference"?

acolytec3 · 2024-09-18T17:39:05Z

packages/blockchain/src/blockchain.ts

+          let updatesHeadBlock
+          if (parentTd === undefined) {
+            // if the block is pow and optimistic, and has not been explicity marked canonical, then
+            // throw error since pow blocks can't be optimistically added without expicit instruction about


I don't think I grasp what this means. How do we know if a PoW block is canonical if we don't have the chain of blocks with successively greater TD leading up to it? Presumably we aren't communicating with a CL client in this instance so not sure where this notion of canonicality would come from external to the blockchain class.

g11tech added PR state: WIP package: blockchain labels Aug 13, 2024

g11tech mentioned this pull request Aug 13, 2024

blockchain: move chain,skeleton to blockchain #3548

Closed

3 tasks

g11tech commented Aug 13, 2024

View reviewed changes

g11tech added 5 commits August 18, 2024 20:07

blockchain: allow optimistic block insertion in blockchain

3b24f64

add and use optimistic number to hash key/dbop to avoid conflict with…

a69d63f

… number to ahash and rebuild canonical while backfilling

add and handle further modifications and flags to handle skeleton fun…

e640849

…ctionality

debug and fix skeleton tests

bbb140b

further debug and fix the beacon sync sim test and get it working

23fd13f

g11tech force-pushed the extend-blockchain2 branch from f1dba27 to 23fd13f Compare August 18, 2024 14:38

holgerd77 reviewed Aug 19, 2024

View reviewed changes

Jamiecaty approved these changes Aug 25, 2024

View reviewed changes

refactor the entire approach to simply allow building normal index as…

c7f2ae2

… optimistic without moving head but track completion by total difficulty index

g11tech added 3 commits September 2, 2024 16:36

modify putopts to be more understanable and add the optimistic block …

d9e3297

…insertion test

add 4444 and optimistic spec debug and get it working

7bd20c2

add 4444 and optimisitc examples

e4253d6

holgerd77 reviewed Sep 16, 2024

View reviewed changes

g11tech commented Sep 16, 2024

View reviewed changes

holgerd77 reviewed Sep 16, 2024

View reviewed changes

jochem-brouwer reviewed Sep 16, 2024

View reviewed changes

remove skipping validation

3bbed08

holgerd77 reviewed Sep 18, 2024

View reviewed changes

acolytec3 reviewed Sep 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blockchain: allow optimistic block insertion in blockchain #3584

blockchain: allow optimistic block insertion in blockchain #3584

g11tech commented Aug 13, 2024

g11tech Aug 13, 2024

holgerd77 Aug 19, 2024

holgerd77 Aug 19, 2024

holgerd77 commented Aug 27, 2024

holgerd77 Sep 16, 2024

holgerd77 Sep 16, 2024

g11tech Sep 16, 2024

holgerd77 Sep 16, 2024

g11tech Sep 16, 2024

holgerd77 Sep 16, 2024

holgerd77 Sep 18, 2024

jochem-brouwer left a comment

jochem-brouwer Sep 16, 2024

jochem-brouwer Sep 16, 2024

g11tech Sep 18, 2024

jochem-brouwer Sep 16, 2024

g11tech Sep 18, 2024

holgerd77 Sep 18, 2024

jochem-brouwer Sep 16, 2024

g11tech Sep 18, 2024

jochem-brouwer Sep 16, 2024

g11tech Sep 18, 2024

jochem-brouwer Sep 16, 2024

g11tech Sep 18, 2024

jochem-brouwer Sep 16, 2024

g11tech Sep 18, 2024

holgerd77 Sep 18, 2024

holgerd77 Sep 18, 2024

g11tech Sep 18, 2024

holgerd77 Sep 18, 2024

acolytec3 Sep 18, 2024

acolytec3 Sep 18, 2024

		@@ -14,6 +14,8 @@ import {

		import type { CacheMap } from './manager.js'

		export type OptimisticOpts = { fcUed: boolean; linked?: boolean }

		@@ -13,6 +13,8 @@ import {

		import type { CacheMap } from './manager.js'

		export type PutOpts = { canonical?: boolean; parentTd?: bigint }

blockchain: allow optimistic block insertion in blockchain #3584

Are you sure you want to change the base?

blockchain: allow optimistic block insertion in blockchain #3584

Conversation

g11tech commented Aug 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holgerd77 commented Aug 27, 2024

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

jochem-brouwer left a 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

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