pass extra bytes directly to precompiles #675
Conversation
| extra := accessibleState.GetBlockContext().Extra() | ||
| predicateResultsBytes := predicate.GetPredicateResultBytes(extra) |
There was a problem hiding this comment.
IMO, the logic in this file from line 63-72, should be moved somewhere else.
ie, this contract is called in the context of some block and some tx, so it should not have to specify the hash of the tx currently being executed to the statedb to query it for the predicates, then query the results from the block header.
There was a problem hiding this comment.
Agreed. I'm happy to review that as part of this PR, or as a later one if you'd like. I'm approving in case you choose the latter so please ping me if you want a re-review.
| // GetPredicateResults returns an byte array result of verifying the predicates | ||
| // of the given block. | ||
| GetPredicateResultsBytes() []byte | ||
| Extra() []byte // Exta returns the Extra field of the block header |
There was a problem hiding this comment.
How about HeaderExtra() []byte and remove the comment? That way it's also implicitly documented at the call site.
| extra := accessibleState.GetBlockContext().Extra() | ||
| predicateResultsBytes := predicate.GetPredicateResultBytes(extra) |
There was a problem hiding this comment.
Agreed. I'm happy to review that as part of this PR, or as a later one if you'd like. I'm approving in case you choose the latter so please ping me if you want a re-review.
| @@ -241,6 +237,6 @@ func (b *BlockContext) Timestamp() uint64 { | |||
| return b.time | |||
| } | |||
|
|
|||
| func (b *BlockContext) GetPredicateResultsBytes() []byte { | |||
| return b.predicateResultsBytes | |||
| func (b *BlockContext) Extra() []byte { | |||
There was a problem hiding this comment.
I think this PR conflicts with my comment here
If we can extend the BlockContext here, why we avoid using full and parsed types here?
| @@ -61,7 +61,8 @@ func handleWarpMessage(accessibleState contract.AccessibleState, input []byte, s | |||
| warpIndex := int(warpIndexInput) // This conversion is safe even if int is 32 bits because we checked above. | |||
| state := accessibleState.GetStateDB() | |||
| predicateBytes, exists := state.GetPredicateStorageSlots(ContractAddress, warpIndex) | |||
| predicateResultsBytes := accessibleState.GetBlockContext().GetPredicateResultsBytes() | |||
| extra := accessibleState.GetBlockContext().Extra() | |||
| predicateResultsBytes := predicate.GetPredicateResultBytes(extra) | |||
There was a problem hiding this comment.
IMHO:
- warp contract should not need to deal with parsing predicate results (we already parse them for verification). I'm looking at the upstream and seems they also don't include header in the block context. If we can be flexible in BlockContext (we should be), we don't actually need to abstract everything. I do think BlockContext should be the place where we can have custom stuff derived from block/header to present for the execution.
- warp contract should not know about extra at all
- Predicaters does not need to be just for warp messaging, any other predicater should not re-implement all this parse-from-extra logic.
These are my observations after looking at just diffs, block context and warp handler. I haven't considered libevm or any other cyclic dependency. LMK if there is another concern that outweighs these considerations (especially parsing).
There was a problem hiding this comment.
This approach #679 tries to follow the comment above, let me know your thoughts.
Why this should be merged
I don't think the libevm shim needs to know about the length of the dynamic fee window, as this is a somewhat leaky abstraction.
IMO it's simpler to give access to the whole extra bytes in the precompile.
The VM maintains the promise to parse this data for any errors before passing them to the precompile, so this has no semantic changes.
How this works
Refactoring (moves call to predicate.GetResultsBytes from libevm shim to precompiles)
How this was tested
CI