Skip to content

Changed hashing schema implementation to match provider-NIT-4724 #4586

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
joshuacolvin0 merged 3 commits into from
Apr 3, 2026
Merged

Changed hashing schema implementation to match provider-NIT-4724 #4586

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1cd583f
Changed hashing schema implementation to match provider
mahdy-nasr Mar 31, 2026
50a91e4
remove unused code
mahdy-nasr Apr 1, 2026
8e59115
Merge branch 'master' into implement-string-hashing-for-address-filte…
joshuacolvin0 Apr 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelog/mnasr-nit-4724.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
### Changed
- Change hashing algorithm for address filtering feature to match the provider specs.
15 changes: 9 additions & 6 deletions execution/gethexec/addressfilter/address_checker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ package addressfilter

import (
"context"
"crypto/sha256"
"sync"
"testing"

"github.com/google/uuid"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

Expand All @@ -23,15 +23,17 @@ func mustState(t *testing.T, s any) *HashedAddressCheckerState {
}

func TestHashedAddressCheckerSimple(t *testing.T) {
salt := []byte("test-salt")
salt, err := uuid.Parse("3ccf0cbf-b23f-47ba-9c2f-4e7bd672b4c7")
require.NoError(t, err, "failed to parse salt UUID")

addrFiltered := common.HexToAddress("0x000000000000000000000000000000000000dead")
addrFiltered := common.HexToAddress("0xddfAbCdc4D8FfC6d5beaf154f18B778f892A0740")
addrAllowed := common.HexToAddress("0x000000000000000000000000000000000000beef")

const cacheSize = 100
store := NewHashStore(cacheSize)

hash := sha256.Sum256(append(salt, addrFiltered.Bytes()...))
// These values are test values from the provider, to cross-check the salting/hashing algorithm.
hash := common.HexToHash("0x8fb74f22f0aed996e7548101ae1cea812ccdf86e7ad8a781eebea00f797ce4a6")
store.Store(salt, []common.Hash{hash}, "test")

checker := NewHashedAddressChecker(store, 4, 8192)
Expand Down Expand Up @@ -78,7 +80,8 @@ func TestHashedAddressCheckerSimple(t *testing.T) {
}

func TestHashedAddressCheckerHeavy(t *testing.T) {
salt := []byte("heavy-salt")
salt, err := uuid.Parse("3ccf0cbf-b23f-47ba-9c2f-4e7bd672b4c7")
require.NoError(t, err, "failed to parse salt UUID")

const filteredCount = 500
const cacheSize = 100
Expand All @@ -88,7 +91,7 @@ func TestHashedAddressCheckerHeavy(t *testing.T) {
for i := range filteredAddrs {
addr := common.BytesToAddress([]byte{byte(i + 1)})
filteredAddrs[i] = addr
filteredHashes[i] = sha256.Sum256(append(salt, addr.Bytes()...))
filteredHashes[i] = HashWithSalt(salt, addr)
}

store := NewHashStore(cacheSize)
Expand Down
33 changes: 16 additions & 17 deletions execution/gethexec/addressfilter/hash_store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"sync/atomic"
"time"

"github.com/google/uuid"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/common/lru"
)
Expand All @@ -16,7 +18,7 @@ import (
// Once created, this struct is never modified, making it safe for concurrent reads.
// The cache is included here so it gets swapped atomically with the hash data.
type hashData struct {
salt []byte
salt uuid.UUID
hashes map[common.Hash]struct{}
digest string
loadedAt time.Time
Expand All @@ -32,6 +34,12 @@ type HashStore struct {
cacheSize int
}

func HashWithSalt(salt uuid.UUID, address common.Address) common.Hash {
hashInput := salt.String() + "::0x" + common.Bytes2Hex(address.Bytes())
Copy link
Copy Markdown
Contributor

@tsahee tsahee Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add to hashData a string that's just salt.String() + "::0x" so we don't recompute it for every computed hash. It could either be instead of the stored salt or in addition to it

saltedHash := sha256.Sum256([]byte(hashInput))
return saltedHash
}

func NewHashStore(cacheSize int) *HashStore {
h := &HashStore{
cacheSize: cacheSize,
Expand All @@ -46,7 +54,7 @@ func NewHashStore(cacheSize int) *HashStore {
// Store atomically swaps in a new hash list.
// This is called after a new hash list has been downloaded and parsed.
// A new LRU cache is created for the new data, ensuring atomic consistency.
func (h *HashStore) Store(salt []byte, hashes []common.Hash, digest string) {
func (h *HashStore) Store(salt uuid.UUID, hashes []common.Hash, digest string) {
newData := &hashData{
salt: salt,
hashes: make(map[common.Hash]struct{}, len(hashes)),
Expand All @@ -65,22 +73,15 @@ func (h *HashStore) Store(salt []byte, hashes []common.Hash, digest string) {
// This method is safe to call concurrently.
func (h *HashStore) IsRestricted(addr common.Address) bool {
data := h.data.Load() // Atomic load - no lock needed
if len(data.salt) == 0 {
if data.salt == uuid.Nil {
return false // Not initialized
}

// Check cache first (cache is per-data snapshot)
if restricted, ok := data.cache.Get(addr); ok {
return restricted
}

saltedAddr := make([]byte, len(data.salt)+common.AddressLength)
copy(saltedAddr, data.salt)
copy(saltedAddr[len(data.salt):], addr.Bytes())
saltedHash := sha256.Sum256(saltedAddr)

_, restricted := data.hashes[saltedHash]

_, restricted := data.hashes[HashWithSalt(data.salt, addr)]
// Cache the result
data.cache.Add(addr, restricted)
return restricted
Expand All @@ -100,14 +101,12 @@ func (h *HashStore) LoadedAt() time.Time {
}

// Salt returns a copy of the current salt.
func (h *HashStore) Salt() []byte {
func (h *HashStore) Salt() uuid.UUID {
data := h.data.Load()
if len(data.salt) == 0 {
return nil
if data.salt == uuid.Nil {
return uuid.Nil
}
salt := make([]byte, len(data.salt))
copy(salt, data.salt)
return salt
return data.salt
}

// CacheLen returns the current number of entries in the LRU cache.
Expand Down
20 changes: 9 additions & 11 deletions execution/gethexec/addressfilter/s3_sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import (
"fmt"
"strings"

"github.com/google/uuid"

"github.com/ethereum/go-ethereum/common"
"github.com/ethereum/go-ethereum/log"

Expand Down Expand Up @@ -67,11 +69,11 @@ func (s *S3SyncManager) handleHashListData(data []byte, digest string) error {
}

// parseHashListJSON parses the JSON hash list file.
// Expected format: {"salt": "hex...", "address_hashes": [{"hash": "hex1"}, {"hash": "hex2"}, ...]}
func parseHashListJSON(data []byte) ([]byte, []common.Hash, error) {
// Expected format: {"salt": "uuid-string-representation", "address_hashes": [{"hash": "hex1"}, {"hash": "hex2"}, ...]}
func parseHashListJSON(data []byte) (uuid.UUID, []common.Hash, error) {
var payload hashListPayload
if err := json.Unmarshal(data, &payload); err != nil {
return nil, nil, fmt.Errorf("JSON unmarshal failed: %w", err)
return uuid.Nil, nil, fmt.Errorf("JSON unmarshal failed: %w", err)
}

// Validate hashing scheme - warn if not Sha256 but continue for forward compatibility
Expand All @@ -80,25 +82,21 @@ func parseHashListJSON(data []byte) ([]byte, []common.Hash, error) {
"scheme", payload.HashingScheme)
}

salt, err := hex.DecodeString(trimHexPrefix(payload.Salt))
salt, err := uuid.Parse(payload.Salt)
Copy link
Copy Markdown
Contributor

@diegoximenes diegoximenes Mar 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to enforce that Salt is an uuid here?
Spec only says it is string.
This can break down the line, salt format can change to not use uuid, which is valid according to the spec, and we will break here.

Copy link
Copy Markdown
Contributor Author

@mahdy-nasr mahdy-nasr Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no detailed actual spec. we have only code example of the hashing. But from the example and from speaking with them. the address is in form of '0x....' with 20bytes but used with delimiter in a string version. and Salt is UUID using with delimiter too in string version.

This implementation is better to catch any deviation and has strong validation that we get the data in right format. we part the 20byte addresses. we parse the 32Byte hashes, we parse the UUID and when do the hashing we use string::string format.

In future, the only change we spoke about that they will do is moving everything to binary bytes for hashing.

Copy link
Copy Markdown
Contributor

@diegoximenes diegoximenes Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specification only says that salt is a string.
The fact that they are using a uuid, and providing the uuid in string format is irrelevant to nitro then.
The provider can decide to change how the salt is built on the fly, and we should be able to handle that gracefully if they provide the string representation.
Just use the string, no need to enforce parsing uuid for the salt here.
Parsing the uuid is not compliant with the spec, and make nitro more fragile.

Copy link
Copy Markdown
Contributor

@diegoximenes diegoximenes Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mahdy-nasr confirmed, with the salt provider, that despite the document that we have access not mentioning this, the salt will be a uuid.

if err != nil {
return nil, nil, fmt.Errorf("invalid salt hex: %w", err)
}
if len(salt) == 0 {
return nil, nil, fmt.Errorf("salt cannot be empty")
return uuid.Nil, nil, err
}

hashes := make([]common.Hash, len(payload.AddressHashes))
for i, h := range payload.AddressHashes {
hashBytes, err := hex.DecodeString(trimHexPrefix(h.Hash))
if err != nil {
return nil, nil, fmt.Errorf("invalid hash hex at index %d: %w", i, err)
return uuid.Nil, nil, fmt.Errorf("invalid hash hex at index %d: %w", i, err)
}
if len(hashBytes) != 32 {
return nil, nil, fmt.Errorf("invalid hash length at index %d: got %d, want 32", i, len(hashBytes))
return uuid.Nil, nil, fmt.Errorf("invalid hash length at index %d: got %d, want 32", i, len(hashBytes))
}
copy(hashes[i][:], hashBytes)
}

return salt, hashes, nil
}
Loading
Loading