-
Notifications
You must be signed in to change notification settings - Fork 683
feat: build a real pending block #3232
base: develop
Are you sure you want to change the base?
Conversation
5581338
to
facad42
Compare
3b3dfbc
to
a61b1bf
Compare
For the reviewers - I have a few notes on design decisions. When I first started this PR about a year ago, David and I went back and forth on how to handle some race conditions.
Like most things in life, my answer is to take the easy way. When a pending block is requested, we clone the state of the vm and executable transaction pool as is, right now, and make a pending block off of it.
We return a pending block that is already outdated and incorrect. There will always be the possibility of the pending block being incorrect, so I don't think it's worth spending too much energy to make sure it's accurate in this race condition. In the real world, requesting a pending block is just one node's opinion on the next block, based off of the transactions in their pool. So it's even less likely that a pending block is correct in the real world.
In this case we still make a pending block based off of the latest block at the time the request is made. If mining has already started as that request is made, because creating the pending block requires mining all of the same transactions that will be included in the already started block, the pending block will finish second to the already started block. We considered pausing the real miner to make sure the pending block returns first, but it seems silly to slow down real mining for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review, I got pulled down a rabbit hole :)
src/chains/ethereum/ethereum/tests/api/eth/getBlockByNumber.test.ts
Outdated
Show resolved
Hide resolved
src/chains/ethereum/ethereum/src/data-managers/block-manager.ts
Outdated
Show resolved
Hide resolved
src/chains/ethereum/ethereum/src/data-managers/block-manager.ts
Outdated
Show resolved
Hide resolved
src/chains/ethereum/ethereum/src/data-managers/block-manager.ts
Outdated
Show resolved
Hide resolved
src/chains/ethereum/ethereum/tests/api/eth/getBlockByNumber.test.ts
Outdated
Show resolved
Hide resolved
src/chains/ethereum/ethereum/tests/api/eth/getBlockByNumber.test.ts
Outdated
Show resolved
Hide resolved
src/chains/ethereum/ethereum/tests/api/eth/getBlockByNumber.test.ts
Outdated
Show resolved
Hide resolved
I don't know if there's other cases where this is going to be problematic, but at least for On ganache/src/chains/ethereum/ethereum/src/api.ts Line 1121 in 83d195b
StateManager.setStateRoot with the state root of the target block. If we pass in pending , this will be a 32 byte 00 buffer.
|
src/chains/ethereum/ethereum/src/data-managers/block-manager.ts
Outdated
Show resolved
Hide resolved
src/chains/ethereum/ethereum/tests/helpers/compare-chain-state.ts
Outdated
Show resolved
Hide resolved
60fb054
to
16d9346
Compare
@@ -29,12 +30,14 @@ export class Block { | |||
protected _common: Common; | |||
protected _rawTransactions: TypedDatabaseTransaction[]; | |||
protected _rawTransactionMetaData: GanacheRawBlockTransactionMetaData[]; | |||
protected _serialized: Buffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious what the real world benefit of storing the _serialized value here is. I'm concerned that, because the block
is mutable, this could result in unexpected results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We serialize the block when we mine it, so this prevents us from having to do it again any time a block's raw data is needed. We need this raw data when fetching a raw block in the block manager.
All of the fields used in serialization are private and aren't currently modified, but I suppose it's true they could be. This seems worthy of adding to a discussion to weigh the risk/reward.
toJSON<IncludeTransactions extends boolean>( | ||
includeFullTransactions: IncludeTransactions | ||
) { | ||
const json = super.toJSON(includeFullTransactions); | ||
json.stateRoot = this.#toJSONStateRootOverride; | ||
json.hash = this.#toJSONHashOverride; | ||
|
||
return json; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like how you've extended the Block
class, but I feel that overriding the toJSON
behaviour is insufficient - can we rather override the properties stateRoot
and hash
, so that we can do things like:
const block: PendingBlock = ...;
assert.strictEqual(block.hash(), AN_EMPTY_HASH);
assert.strictEqual(block.stateRoot, A_NULL_STATEROOT);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are overriding the toJSON function because we still need the actual stateRoot
and hash
values of the pending block for use cases that traverse the trie. For example, if we were to override the stateRoot
, this function in your eth_getProof
RPC method wouldn't work:
await stateManagerCopy.setStateRoot(
targetBlock.header.stateRoot.toBuffer()
);
However, geth returns empty data for the toJSON
representation of the pending block, hence this "half-measure" override of the data.
This change expands
eth_getBlockByNumber
so that Ganache actually returns apending
block when you call it and pass the "pending" tag. shhhhh, don't tell anyone but in the past we'd just return thelatest
block.So, if you retrieve the
pending
block, Ganache will now return a block that is after thelatest
block and which contains the transactions that are still waiting in the transaction pool to be mined. It's almost like we can see into theSadly our clairvoyance does have its limitations, so don't expect transactions that haven't made it into the transaction pool yet to be in the
pending
block.Fixes #772, #3481, #3529