feature: Support for Hedera EVM block identifiers #118
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,22 @@ | ||
| { | ||
| // Use IntelliSense to learn about possible attributes. | ||
| // Hover to view descriptions of existing attributes. | ||
| // For more information, visit: https://go.microsoft.com/fwlink/?linkid=830387 | ||
| "version": "0.2.0", | ||
| "configurations": [ | ||
| { | ||
| "name": "Run evmconnect", | ||
| "type": "go", | ||
| "request": "launch", | ||
| "mode": "auto", | ||
| "program": "${workspaceFolder}/evmconnect/main.go", | ||
| "args": [ | ||
| "-f", | ||
| "${workspaceFolder}/evmconnect_config.yml" | ||
| ], | ||
| "env": { | ||
| "FIREFLY_PERSISTENCE_LEVELDB_PATH": "${workspaceFolder}/.leveldb" | ||
| } | ||
| } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -39,16 +39,18 @@ type blockUpdateConsumer struct { | |||||
| // 1) To establish and keep track of what the head block height of the blockchain is, so event streams know how far from the head they are | ||||||
| // 2) To feed new block information to any registered consumers | ||||||
| type blockListener struct { | ||||||
| ctx context.Context | ||||||
| c *ethConnector | ||||||
| listenLoopDone chan struct{} | ||||||
| initialBlockHeightObtained chan struct{} | ||||||
| highestBlock int64 | ||||||
| mux sync.Mutex | ||||||
| consumers map[fftypes.UUID]*blockUpdateConsumer | ||||||
| blockPollingInterval time.Duration | ||||||
| unstableHeadLength int | ||||||
| canonicalChain *list.List | ||||||
| allowNonStandardBlockHashLength bool | ||||||
| nonStandardBlockHashSizeResolutionMethod string | ||||||
| } | ||||||
|
|
||||||
| type minimalBlockInfo struct { | ||||||
|
|
@@ -59,14 +61,16 @@ type minimalBlockInfo struct { | |||||
|
|
||||||
| func newBlockListener(ctx context.Context, c *ethConnector, conf config.Section) *blockListener { | ||||||
| bl := &blockListener{ | ||||||
| ctx: log.WithLogField(ctx, "role", "blocklistener"), | ||||||
| c: c, | ||||||
| initialBlockHeightObtained: make(chan struct{}), | ||||||
| highestBlock: -1, | ||||||
| consumers: make(map[fftypes.UUID]*blockUpdateConsumer), | ||||||
| blockPollingInterval: conf.GetDuration(BlockPollingInterval), | ||||||
| canonicalChain: list.New(), | ||||||
| unstableHeadLength: int(c.checkpointBlockGap), | ||||||
| allowNonStandardBlockHashLength: conf.GetBool(AllowNonStandardBlockHashSize), | ||||||
| nonStandardBlockHashSizeResolutionMethod: conf.GetString(NonStandardBlockHashSizeResolutionMethod), | ||||||
| } | ||||||
| return bl | ||||||
| } | ||||||
|
|
@@ -140,6 +144,28 @@ func (bl *blockListener) listenLoop() { | |||||
| update := &ffcapi.BlockHashEvent{GapPotential: gapPotential} | ||||||
| var notifyPos *list.Element | ||||||
| for _, h := range blockHashes { | ||||||
| if len(h) != 32 { | ||||||
|
There was a problem hiding this comment. Sorry @SamMayWork I couldn't understand in the end the problem and the solution. Could do with some help. 32 is the length of a "standard" Ethereum block hash. So if we get a standard one, it will be 32 bytes in length. You are working with a EVM implementation against a ledger that uses a 48 byte block hash - so in that case, the length here would be 48. Why would we truncate that? Surely we just need to acommodate that it's 48 bytes, and there will be some number of places in the code that need to be relaxed to avoid the assumption of 32 bytes. Maybe for example there are DB migration tables etc. There was a problem hiding this comment.
There was a brief Discord discussion on this, although in retrospect I don't think we considered what other alternative approaches we could use here. The reason for using truncation here instead of changing the code everywhere to permit the change in header size was purely to reduce the scope of the changes required. I think the view was that Hedera is EVM compatible but also a little bit weird, and it might make more sense to truncate the hashes so there's no need to change existing code for a single use-case. We could use this PR to permit non-standard block header sizes everywhere if that's something strategically we think EVM Connect should be able to do. There was a problem hiding this comment. Had a conversation with @peterbroadhurst on this topic, one thing this PR doesn't call out that it absolutely should be is that it is expected in the Hedera ecosystem to refer to blocks by their truncated hash. Block headers are 48 bytes, truncation down to 32 bytes for EVM compatiblity is baked into the Hedera ecosystem, so this isn't a case of FireFly doing something strange, this is what we're expected to be doing. Additionally on the spelling of the configuration we've agreed that in this case we're processing blocks slightly differently specifically for Hedera so in that sense we're essentially running compatibility adjustments, as such we've change the name of the configuration to be There was a problem hiding this comment.
Suggested change
|
||||||
| if !bl.allowNonStandardBlockHashLength { | ||||||
| log.L(bl.ctx).Errorf("Tried to index a non-standard size block hash length: %s", h.String()) | ||||||
| failCount++ | ||||||
| continue | ||||||
| } | ||||||
|
|
||||||
| if bl.nonStandardBlockHashSizeResolutionMethod == "" { | ||||||
| log.L(bl.ctx).Warnf("No non-standard hash length resolution method was provided, defaulting to truncation") | ||||||
| bl.nonStandardBlockHashSizeResolutionMethod = "truncate" | ||||||
| } | ||||||
|
|
||||||
| if bl.nonStandardBlockHashSizeResolutionMethod == "truncate" { | ||||||
| h, err = h.Truncate(32) | ||||||
|
There was a problem hiding this comment.
Suggested change
... related to firefly-signer change proposal. But note the question overall around the intent of this change. There was a problem hiding this comment. Yep agreed here - if we go with this approach, we'll do this and then remove the requirement on the firefly-signer PR. |
||||||
| if err != nil { | ||||||
| log.L(bl.ctx).Errorf("Input hash was shorter than the standard block hash length: %s", h.String()) | ||||||
| failCount++ | ||||||
| continue | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Do a lookup of the block (which will then go into our cache). | ||||||
| bi, err := bl.c.getBlockInfoByHash(bl.ctx, h.String()) | ||||||
| switch { | ||||||
|
|
||||||
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.
Will be removed after the pre-req changes are merged
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.
Please see review comments on hyperledger/firefly-signer#61