feature: Support for Hedera EVM block identifiers #118

SamMayWork · 2024-03-07T16:29:32Z

ref: https://discord.com/channels/905194001349627914/928377875827154984/1219628950393847860 (thread on Hyperledger discord on Hedera support with EVM Connect)

TL;DR: Hedera is a hashgraph not a blockchain (though is EVM compatible), and uses non-standard 'block' hashes which are 48 bytes rather than 32 bytes, EVM Connect cannot handle this currently. To promote integration with other ecosystems, Hedera expects that these 48 byte block headers be truncated to 32 bytes, this is a common feature in the Hedera ecosystem.

https://hips.hedera.com/hip/hip-415 - Hedera introduction of blocks (mentioning 48 bytes being referenced by the 32 byte prefix)

https://hips.hedera.com/hip/hip-482 - JSON RPC specification for Hedera

Block Hashes

Ethereum uses 32-byte hashes for blocks while Hedera uses 48-byte hashes. To compute a 32-byte Ethereum hash, take the leading 32 bytes from a 48-byte Hedera hash.

Signed-off-by: SamMayWork <[email protected]>

SamMayWork · 2024-03-25T11:59:24Z

go.mod

@@ -99,3 +99,5 @@ require (
 	gopkg.in/natefinch/lumberjack.v2 v2.2.1 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
 )
+
+replace github.com/hyperledger/firefly-signer => github.com/SamMayWork/firefly-signer v0.0.0-20240307121035-e654fb9ef55b


Will be removed after the pre-req changes are merged

Please see review comments on hyperledger/firefly-signer#61

Signed-off-by: SamMayWork <[email protected]>

peterbroadhurst

Need a little more info on the solution you're proposing, and the alternatives you considered @SamMayWork if that's ok

peterbroadhurst · 2024-05-10T12:22:17Z

internal/ethereum/blocklistener.go

+				}
+
+				if bl.nonStandardBlockHashSizeResolutionMethod == "truncate" {
+					h, err = h.Truncate(32)


Suggested change

h, err = h.Truncate(32)

h = ethtypes.HexBytes0xPrefix(h[0:32])

... related to firefly-signer change proposal. But note the question overall around the intent of this change.

Yep agreed here - if we go with this approach, we'll do this and then remove the requirement on the firefly-signer PR.

peterbroadhurst · 2024-05-10T12:25:32Z

internal/msgs/en_config_descriptions.go

+	ConfigTxCacheSize                              = ffc("config.connector.txCacheSize", "Maximum of transactions to hold in the transaction info cache", i18n.IntType)
+	ConfigMaxConcurrentRequests                    = ffc("config.connector.maxConcurrentRequests", "Maximum of concurrent requests to be submitted to the blockchain", i18n.IntType)
+	ConfigAllowNonStandardBlockHashSize            = ffc("config.connector.allowNonStandardBlockHashSize", "Whether to permit block hases of a non-standard (i.e. not 32 bytes) length", i18n.BooleanType)
+	ConfigNonStandardBlockHashSizeResolutionMethod = ffc("config.connector.nonStandardBlockHashSizeResolutionMethod", "How to resolve block hashes that are non-standard (i.e. not 32 bytes) in length. Currently only 'truncate' is supported.", i18n.StringType)


I'm not sure I understand yet the intention here, and that suggests to me users of this setting might struggle to.

Is the intention here blockHashByteLength?

e.g. you are instructing the whole code stack that all block hashes should be X bytes, where the default is 32?

If so, what code is affected that needs this? And why?

Will try and answer from reading through the rest of the code

peterbroadhurst · 2024-05-10T12:28:28Z

internal/ethereum/blocklistener.go

@@ -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 {


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.

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 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.

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 hederaCompatibilityMode.

Suggested change

if len(h) != 32 {

if len(h) == 48 {

Signed-off-by: SamMayWork <[email protected]>

peterbroadhurst · 2024-05-20T13:35:25Z

fix: Allow for non-standard block header lengths

Maybe worth changing to:

"feature: Support for Hedera EVM block identifiers"

peterbroadhurst · 2024-05-20T13:37:33Z

internal/ethereum/blocklistener.go

+					continue
+				}
+
+				h = h[0:32]


Note above to make this safe we need to be confident we have at least 32b

peterbroadhurst

Minor suggestion to tweak the length check before merge, and a request to change the PR title for the release when that happens.

Signed-off-by: SamMayWork <[email protected]>

EnriqueL8

Approving and merging based on above approvals!

fix: allow for bytes to be truncated down

88794b1

Signed-off-by: SamMayWork <[email protected]>

SamMayWork changed the title ~~fix: allow for bytes to be truncated down~~ fix: Allow for non-standard block header lengths Mar 19, 2024

SamMayWork added 2 commits March 21, 2024 16:05

merge

5a3a391

Signed-off-by: SamMayWork <[email protected]>

feat: allow for support for non-standard block hash lengths

7fbecb3

Signed-off-by: SamMayWork <[email protected]>

SamMayWork marked this pull request as ready for review March 25, 2024 11:40

SamMayWork requested a review from a team as a code owner March 25, 2024 11:40

fix: revert to normal launch json

388881d

Signed-off-by: SamMayWork <[email protected]>

SamMayWork mentioned this pull request Mar 25, 2024

feat: Allow for truncation of 0x prefix hashes hyperledger/firefly-signer#61

Closed

SamMayWork commented Mar 25, 2024

View reviewed changes

merge

f6e6bc2

Signed-off-by: SamMayWork <[email protected]>

peterbroadhurst requested changes May 10, 2024

View reviewed changes

SamMayWork added 4 commits May 10, 2024 16:07

fix: change to more obvious spelling

7717d19

Signed-off-by: SamMayWork <[email protected]>

merge

266e6bd

Signed-off-by: SamMayWork <[email protected]>

fix: missed one

aa8644a

Signed-off-by: SamMayWork <[email protected]>

fix: drop signer changes

7d84b1d

Signed-off-by: SamMayWork <[email protected]>

peterbroadhurst reviewed May 20, 2024

View reviewed changes

peterbroadhurst approved these changes May 20, 2024

View reviewed changes

SamMayWork changed the title ~~fix: Allow for non-standard block header lengths~~ feature: Support for Hedera EVM block identifiers May 20, 2024

fix: safety

bf4fe5c

Signed-off-by: SamMayWork <[email protected]>

SamMayWork force-pushed the use-truncate branch from ae14a73 to bf4fe5c Compare May 20, 2024 14:06

EnriqueL8 approved these changes May 20, 2024

View reviewed changes

EnriqueL8 merged commit 684d91e into hyperledger:main May 20, 2024
3 checks passed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Support for Hedera EVM block identifiers #118

feature: Support for Hedera EVM block identifiers #118

SamMayWork commented Mar 7, 2024 •

edited

Loading

SamMayWork Mar 25, 2024

peterbroadhurst May 10, 2024

peterbroadhurst left a comment

peterbroadhurst May 10, 2024 •

edited

Loading

SamMayWork May 10, 2024

peterbroadhurst May 10, 2024

peterbroadhurst May 10, 2024

SamMayWork May 10, 2024

SamMayWork May 10, 2024

peterbroadhurst May 20, 2024

peterbroadhurst commented May 20, 2024

peterbroadhurst May 20, 2024

peterbroadhurst left a comment

EnriqueL8 left a comment

	h, err = h.Truncate(32)
	h = ethtypes.HexBytes0xPrefix(h[0:32])

feature: Support for Hedera EVM block identifiers #118

feature: Support for Hedera EVM block identifiers #118

Conversation

SamMayWork commented Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterbroadhurst left a comment

Choose a reason for hiding this comment

peterbroadhurst May 10, 2024 • edited Loading

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

peterbroadhurst commented May 20, 2024

Choose a reason for hiding this comment

peterbroadhurst left a comment

Choose a reason for hiding this comment

EnriqueL8 left a comment

Choose a reason for hiding this comment

SamMayWork commented Mar 7, 2024 •

edited

Loading

peterbroadhurst May 10, 2024 •

edited

Loading