feat(vms/evm/sync/code): add code-by-hash request handler

[powerslider]

Why this should be merged

Check #5401

How this works

• Registers a code-by-hash handler at p2p.EVMCodeRequestHandlerID. The package exposes just registering a handler and handing a client to the syncer.
• One goroutine drains the queue, checks the db, and groups hashes into batches, so a hash is claimed exactly once. Each full batch is fetched, verified, and persisted by a worker, bounded to numSyncWorkers at a time. A hash repeated while its fetch is outstanding is not fetched again.
• The client verifies every response before accepting it: exact count, size bound, and each blob hashes to its requested hash. A mismatch scores the peer down and re-requests, so bad code can never land.
• A blob and its to-fetch marker are written in one batch, so a crash cannot leave code stored with its marker still set.
• Caps hashes per request off the p2p message limit instead of a fixed 5, and serves duplicate hashes rather than dropping the whole request. A batch ships the moment it fills, so a request can never outgrow what the peer accepts.
• A request the server refuses now carries an AppError rather than being dropped, so the client fails over to another peer immediately instead of waiting out a timeout. Code sync reserves the 1000 range for its sentinels.

How this was tested

UTs plus integration tests that run the syncer against the real handler over an in-process network.

Need to be documented in RELEASES.md?

no

resolves #5401

Signed-off-by: Tsvetan Dimitrov (tsvetan.dimitrov@avalabs.org)

[alarso16]

IMO we shouldn't migrate this, but just tighten the client.Client interface to the only method that's used GetCode, and we can fully migrate it later

[alarso16]

My suggestion is to follow something closer to #5731, to speed up review and merge to get state sync out the door

[alarso16]

A few nits, looks good to me

[Copilot]

Pull request overview

Adds EVM “code-by-hash” sync plumbing: a p2p request handler for serving contract bytecode by hash, plus a syncer that batches requested hashes, verifies responses, and persists code into the local DB. This supports the broader EVM state sync workflow by enabling peers to fetch missing contract code blobs safely.

Changes:

• Added a p2p.EVMCodeRequestHandlerID handler to serve code blobs by requested hashes with size/limit checks.
• Added a code syncer that consumes code hashes, batches requests, verifies responses, and writes code + clears “to fetch” markers.
• Added UTs and an in-process integration-style test using a new synctest.NewSelfNetwork helper.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
vms/evm/sync/synctest/network.go	Adds a single-node loopback p2p network helper used by sync tests.
vms/evm/sync/synctest/BUILD.bazel	Updates Bazel deps for the enhanced synctest network helper.
vms/evm/sync/code/syncer.go	Introduces the code syncer (batching, verification, persistence).
vms/evm/sync/code/syncer_test.go	Adds syncer unit tests plus a tampered-response retry/verification test.
vms/evm/sync/code/handler.go	Adds the code-by-hash request handler and request-size cap.
vms/evm/sync/code/handler_test.go	Adds handler tests (order, duplicates, missing hash, limits, sentinel uniqueness).
vms/evm/sync/code/BUILD.bazel	Expands library sources and adds Bazel tests for the new code sync components.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[StephenButtolph]

We should comment why this is required. I suspect it isn't, although I feel like it does prevent us from doing some more work than we need to.

For instance, if we requested a bunch of big contracts, it may be that essentially the max message size of bytes is returned by the peer. And then we would hash all of those code snippets (with the last one potentially failing). So in this case we could still end up hashing up to the max message size worth of bytes.

However, that assumed that we queried a bunch of big contracts... In practice the request is going to be of a bunch of contracts picked by us (the client) - so a malicious node would only be able to force us to hash the size of the actual expected results + 1 max size contract at the end.

[powerslider]

Yeah, it is not required. An oversized blob cannot hash to what we asked for, so the hash check alone rejects it. The size check is purely about bounding the work. #5654 fixes that separately by moving the retry loop into the Dispatcher with a backoff on peer-scoped failures, which is where the pacing belongs. Comment change here e198c32#r3729919331

[StephenButtolph]

These are only set in tests, and the tests that set them don't fail if we stop setting them. Feels like we can just get rid of these.

[StephenButtolph]

These comments are unusual. The Fast path happening after the Slow path is weird. It makes it seem like it was an optimistic check, but with the wrong order.

[powerslider]

This is not relevant anymore, but it worked the following way: the two checks were ordered by cost, not by optimism. The disk read came first because a hash whose code is already stored needs no network work at all, only its marker cleared. The in-flight check came second because it only matters for a hash we are actually going to fetch. So "slow" and "fast" were describing the check itself, a disk read versus a map lookup, rather than the path through the function.

But again now I've redesigned the flow via the manager goroutine mechanism you proposed.

[StephenButtolph]

Removing this branch doesn't cause the test to fail, so I'm not sure that these are really asserting the behavior they are intending to.

[alarso16]

I was looking at your code and thought it was more complicated than necessary, and that we MUST parallelize DB writes, so I opened #5781 with some ideas to share. Feel free to use it (or don't)

[powerslider]

@alarso16 used some of your ideas and implemented them. Yes, using SetLimit on the errgroup does eliminate some of the machinery that I used and reduced the code needed. Check here: 2f5d019

[alarso16]

I like this a lot better

[alarso16]

Should you be returning the batch? It seems like if the context is canceled, we won't request it

[alarso16]

Suggested change

	type claimSet struct {
	mu sync.Mutex
	hashes map[common.Hash]struct{}
	}
	func newClaimSet() *claimSet {
	return &claimSet{
	hashes: make(map[common.Hash]struct{}),
	}
	}
	// claim reports whether codeHash was taken, and false if it was already held.
	func (c *claimSet) claim(codeHash common.Hash) bool {
	c.mu.Lock()
	defer c.mu.Unlock()
	if _, dup := c.hashes[codeHash]; dup {
	return false
	}
	c.hashes[codeHash] = struct{}{}
	return true
	}
	func (c *claimSet) release(hashes []common.Hash) {
	c.mu.Lock()
	defer c.mu.Unlock()
	for _, codeHash := range hashes {
	delete(c.hashes, codeHash)
	}
	}
	type claimSet struct {
	m sync.Map
	}
	func newClaimSet() *claimSet {
	return &claimSet{
	m: new(sync.Map),
	}
	}
	// claim reports whether codeHash was taken, and false if it was already held.
	func (c *claimSet) claim(codeHash common.Hash) bool {
	_, alreadyClaimed := c.m.LoadOrStore(codeHash, struct{}{})
	return !alreadyClaimed
	}
	func (c *claimSet) release(hashes []common.Hash) {
	for _, codeHash := range hashes {
	c.m.Delete(codeHash)
	}
	}

As an option, I think you can use any atomic map safely (note: this code may not compile)