xdc eth endpoints

[ak88]

Missing eth_ endpoints for XDC.

[claude]

Claude finished @ak88's task in 5m 24s —— View job

---

PR Review: xdc eth endpoints

• Gather context and read changed files
• Review new RPC module and interface
• Review reward store changes
• Review tests
• Post findings

---

Summary

This PR adds three new eth_* RPC endpoints to the XDC module (eth_getOwnerByCoinbase, eth_getRewardByHash, eth_getTransactionAndReceiptProof), introduces a parallel JSON-serialised reward storage path (SaveEpochRewardsRpc), and hooks it all into the reward calculator and DI container. The structure is clean and the proof generation logic looks correct. Two issues should be fixed before merge.

---

Findings

High

1. eth_getRewardByHash stores/retrieves rewards by block number, not hash — XdcExtendedEthModule.cs:50
The method is described as "Returns epoch reward distribution for the block identified by hash," but TryGetEpochRewards is keyed by (ulong)header.Number. In a reorg scenario, calling with a non-canonical block hash returns rewards for the canonical block at that number — violating the API contract. See inline comment for options. Inline comment →

2. SaveEpochRewardsRpc is not atomic with SaveEpochRewards — RewardsStore.cs:83
SaveEpochRewards uses an IWriteBatch (atomic). SaveEpochRewardsRpc issues a separate _rewardsDb.Set(...) outside any batch. A crash between the two leaves HasEpochRewards = true but TryGetEpochRewards = false, causing eth_getRewardByHash to silently return an empty dict. Both writes should be committed in the same batch. Inline comment →

---

Medium

3. eth_getRewardByHash returns Success([]) when block is not found — XdcExtendedEthModule.cs:47
Callers cannot distinguish "unknown block hash" from "epoch block exists but has no rewards." Should return a Fail(...) result. Inline comment →

4. No tests for eth_getTransactionAndReceiptProof with a valid transaction — XdcExtendedEthModuleTests.cs
Only the not-found path is covered. The proof format (key encoding, FromProofNodes output, root hash computation) is entirely untested. Given this is custom proof format code, at least one round-trip test with a real block and known transaction should be added.

5. block.Hash! null suppression — XdcExtendedEthModule.cs:103
If FindBlock ever returns a block without a precomputed hash, this propagates a null reference silently. Prefer block.Hash ?? throw new InvalidOperationException(...). Inline comment →

---

Low

6. IsSharable = true on all three methods without thread-safety documentation
Marking methods as sharable asserts concurrent call safety. The thread-safety guarantees of IMasternodeVotingContract and IRewardsStore implementations should be confirmed (or the attribute set to false until verified).

7. FromProofNodes — misleading variable name keys — XdcExtendedEthModule.cs:118
keys[i] is Keccak.Compute(rlp).ToString() — a node hash, not a trie path key. nodeHashes would be clearer. Inline comment →

8. Pruning of RPC rewards not exercised in tests — RewardsStoreTests.cs
SaveEpochRewards_WhenEpochIsOutsideRetentionWindow_ShouldPruneOlderEntries never calls SaveEpochRewardsRpc, so the batch.Remove(BuildEpochRewardsRpcKey(...)) path is dead code in the test suite. Add a variant that seeds RPC rewards and verifies they are pruned.

[claude]

High — rewards retrieved by block number, not block hash

The method contract says "Returns epoch reward distribution for the block identified by hash," but the lookup is keyed by header.Number, not by the input hash:

if (!rewardsStore.TryGetEpochRewards((ulong)header.Number, out ...))

RewardsStore indexes its data by block number (see BuildEpochRewardsRpcKey). If a chain reorganisation replaces the block at this number, TryGetEpochRewards returns the rewards for the new canonical block — not for the block whose hash was passed in. The caller cannot detect the mismatch.

Options:

1. Store rewards by block hash (and add a hash → number index) so that each call is truly hash-scoped.
2. At minimum, after the lookup, verify that the canonical block at header.Number still has the same hash, and return an error or empty if it doesn't. Fix this →

[claude]

Medium — silent "not found" masquerades as "no rewards"

When FindHeader(blockHash) returns null the method returns Success([]), which is indistinguishable from a genuine "epoch block exists but has zero reward entries." Callers that pass an invalid or pruned hash will silently receive an empty dict instead of an error.

Suggested change

	return Task.FromResult(ResultWrapper<Dictionary<string, Dictionary<string, Dictionary<string, string>>>>.Success([]));
	return Task.FromResult(ResultWrapper<Dictionary<string, Dictionary<string, Dictionary<string, string>>>>.Fail($"Block with hash {blockHash} not found."));

[claude]

High — SaveEpochRewardsRpc is not atomic with the enclosing write batch in SaveEpochRewards

SaveEpochRewards commits its write batch (sequence keys, prune tombstones, regular reward bytes) atomically. SaveEpochRewardsRpc then issues a separate _rewardsDb.Set(...) outside any batch. A process crash between the two writes leaves the store in an inconsistent state:

• HasEpochRewards → true (regular reward key exists)
• TryGetEpochRewards → false / empty (RPC key missing)

eth_getRewardByHash returns an empty dict in this case, making it impossible to distinguish "pruned" from "crash-interrupted write."

The fix is to fold the RPC key write into the same IWriteBatch that SaveEpochRewards already creates, either by accepting it as a parameter or by extracting a combined SaveEpochRewardsFull(ulong, BlockReward[], Dictionary<...>) method.

Fix this →

[claude]

Medium — null-forgiving ! on block.Hash

block.Hash is typed as Hash256?. Using ! suppresses the nullable warning. FindBlock should always return a fully hashed block in practice, but if it doesn't, this silently propagates a null reference into XdcTransactionAndReceiptProof.BlockHash, which will later fail in an uncontrolled way.

Suggested change

	BlockHash = block.Hash!,
	BlockHash = block.Hash ?? throw new InvalidOperationException($"Block returned by FindBlock has a null hash for block hash lookup {blockHash}"),

[claude]

Low — misleading variable name

keys[i] is set to Keccak.Compute(rlp).ToString() — it holds the hash of the proof node, not a trie path key. Rename to nodeHashes (or proofNodeHashes) to avoid confusion with the trie-index key stored in XdcTransactionAndReceiptProof.Key.

Suggested change

	string[] keys = new string[proofNodes.Length];
	string[] nodeHashes = new string[proofNodes.Length];

[ak88]

@claude review

[claude]

Claude finished @ak88's task in 3m 7s —— View job

---

PR Review: xdc eth endpoints

• Load tools
• Read changed files
• Review new RPC module and interface
• Review reward store changes
• Review tests
• Post findings

---

Summary

This PR adds three new eth_* RPC endpoints to the XDC module. Since the prior review, commit 8c44a78 (unify rewards format) addressed the High atomicity issue by removing SaveEpochRewardsRpc and folding the JSON write into the single SaveEpochRewards batch — that's a clean fix. The remaining issues from the prior review are still present.

Fixed since last review:

• ✅ High: SaveEpochRewardsRpc non-atomic write — unified into single IWriteBatch in SaveEpochRewards
• ✅ Low: Pruning of RPC rewards not exercised — no longer applicable with unified write path

Still outstanding:

---

High

1. eth_getRewardByHash retrieves by block number, not hash — XdcExtendedEthModule.cs:50
TryGetEpochRewards is keyed by header.Number. In a reorg, two blocks share the same number and the wrong rewards are returned. The method description says "identified by hash" — this is a contract violation. After finding the header, verify the canonical block at that number still has the same hash; return Fail if not. Inline comment →

---

Medium

2. eth_getRewardByHash returns Success([]) for unknown hash — XdcExtendedEthModule.cs:47
Callers cannot distinguish "unknown block hash" from "epoch block has no rewards." Should return Fail(...). Inline comment →

3. block.Hash! null suppression — XdcExtendedEthModule.cs:103
Block.Hash is Hash256?. The ! silently propagates null into XdcTransactionAndReceiptProof.BlockHash. Prefer ?? throw new InvalidOperationException(...). Inline comment →

4. No round-trip test for eth_getTransactionAndReceiptProof with a valid transaction
Only the not-found path is covered. The proof format (key encoding, FromProofNodes output, root hash) is entirely untested. Add at least one test with a real block and a known transaction index.

---

Low

5. IsSharable = true on all three methods without thread-safety documentation
Marking methods as sharable asserts concurrent call safety. Confirm that IMasternodeVotingContract and IRewardsStore implementations are safe for concurrent RPC dispatch, or set to false until confirmed.

6. Misleading variable name keys — XdcExtendedEthModule.cs:118
keys[i] stores Keccak.Compute(rlp).ToString() — a node hash, not a trie path key. nodeHashes would be clearer. Inline comment →

[claude]

High — rewards are retrieved by block number, not block hash

TryGetEpochRewards is keyed by header.Number. In a reorg scenario, two different blocks can share the same number, and this returns rewards for whichever number was stored last — not for the specific block whose hash was passed in. The method name and its description ("Returns epoch reward distribution for the block identified by hash") make this a contract violation.

Minimal fix: after the lookup, verify the canonical block at that number still has the same hash:

BlockHeader? canonical = blockFinder.FindHeader(new BlockParameter(header.Number));
if (canonical?.Hash != blockHash)
{
    return Task.FromResult(ResultWrapper<...>.Fail($"Block {blockHash} is not canonical at number {header.Number}."));
}

XDC's consensus makes deep reorgs unlikely, but the API should honour what it declares.

[claude]

Medium — unknown block hash returns Success([]), indistinguishable from "no rewards"

Callers cannot tell whether the empty dict means "unknown hash" or "epoch block had no reward entries." Use Fail here:

Suggested change

	return Task.FromResult(ResultWrapper<Dictionary<string, Dictionary<string, Dictionary<string, string>>>>.Success([]));
	return Task.FromResult(ResultWrapper<Dictionary<string, Dictionary<string, Dictionary<string, string>>>>.Fail($"Block with hash {blockHash} not found."));

[ak88]

@claude review again

[claude]

Claude finished @ak88's task in 2m 39s —— View job

---

PR Review: xdc eth endpoints

• Load tools
• Read changed files
• Review new RPC module and interface
• Review reward store changes
• Review tests
• Post findings

---

Summary

Good progress since the last review. The two previous High findings are now resolved:

• ✅ eth_getRewardByHash now looks up rewards by block hash — IRewardsStore signature changed to TryGetEpochRewards(Hash256, ...), RewardsStore indexes by hash, and the new SaveEpochRewards_WhenSameNumberHasNewHash_ShouldReplaceOldRewardEntry test exercises the reorg-replacement path.
• ✅ block.Hash! null suppression replaced with ?? throw new InvalidOperationException(...).

One new Critical issue was introduced by an incomplete variable rename, and two Medium findings from prior reviews remain unaddressed.

---

Critical

1. FromProofNodes won't compile — keys is undefined — XdcExtendedEthModule.cs:123,127

The variable was renamed to nodeHashes on line 118 but lines 123 and 127 still reference the old name keys. This is CS0103 — the project cannot build. Inline comment (line 123) → Inline comment (line 127) → Fix this →

---

Medium

2. eth_getRewardByHash returns Success([]) for unknown hash — XdcExtendedEthModule.cs:47

Flagged in both prior reviews, still unaddressed. Callers cannot distinguish "unknown block hash" from "epoch block has zero reward entries." Inline comment → Fix this →

3. No round-trip test for eth_getTransactionAndReceiptProof with a valid transaction

Flagged in both prior reviews, still unaddressed. Only the not-found path is covered. The proof format (key encoding, FromProofNodes output, root hash computation) is entirely untested. Add at least one test with a real block and a known transaction index.

---

Low

4. IsSharable = true on all three methods without confirmed thread-safety

Flagged in both prior reviews. Marking methods sharable asserts concurrent call safety. Confirm the thread-safety guarantees of IMasternodeVotingContract and IRewardsStore (both use _rewardsDb.Get / contract calls without locks), or set to false until verified.

[claude]

Critical — keys is not declared; this is a compile error

The variable was renamed to nodeHashes on line 118 but lines 123 and 127 still reference the old name keys. This produces CS0103: The name 'keys' does not exist in the current context.

Suggested change

	keys[i] = Keccak.Compute(rlp).ToString();
	nodeHashes[i] = Keccak.Compute(rlp).ToString();

[claude]

Critical — keys is not declared here either

Same incomplete rename: keys was never declared (it was renamed to nodeHashes).

Suggested change

	return (keys, values);
	return (nodeHashes, values);

[claude]

Medium — unknown hash returns Success([]), indistinguishable from "epoch exists but has no rewards"

This was flagged in both prior reviews and is still unaddressed. Callers that pass a wrong or pruned hash receive an empty dict with no way to tell it apart from a legitimate empty-rewards epoch.

Suggested change

	return Task.FromResult(ResultWrapper<Dictionary<string, Dictionary<string, Dictionary<string, string>>>>.Success([]));
	return Task.FromResult(ResultWrapper<Dictionary<string, Dictionary<string, Dictionary<string, string>>>>.Fail($"Block with hash {blockHash} not found."));

[ak88]

this matches the behavior in XDC client

[ak88]

@claude review

[claude]

Claude finished @ak88's task in 2m 15s —— View job

---

PR Review: xdc eth endpoints

• Load tools
• Read changed files
• Review findings
• Post results

---

Summary

Good progress. The Critical compile error from the previous review (keys undefined after the partial rename) is now fixed — nodeHashes is used consistently throughout FromProofNodes. All prior High findings have been resolved across the review series.

Fixed since last review:

• ✅ Critical: FromProofNodes compile error — nodeHashes now used correctly on all 3 lines

Acknowledged with rationale (not blocking):

• eth_getRewardByHash returning Success([]) for unknown hash — ak88 confirmed this matches XDC client behavior

---

Medium

1. No round-trip test for eth_getTransactionAndReceiptProof with a valid transaction — flagged in 4 reviews, still unaddressed

Only the not-found path is covered. The proof format (key RLP-encoding, FromProofNodes node hashing, TxRoot/ReceiptRoot derivation) is entirely untested. Given this is custom proof-format code, at least one test with a real block and a known transaction is needed to verify correctness. Inline comment →

---

Low

2. header.Hash! null-forgiving operator — XdcExtendedEthModule.cs:51

The header returned by FindHeader(blockHash) should always carry its hash, but the ! hides a potential null. The same pattern was already fixed for block.Hash in this PR. Prefer header.Hash ?? blockHash as a safe fallback. Inline comment →

3. IsSharable = true on all three methods without confirmed thread-safety

Flagged in 3 prior reviews. Marking methods sharable asserts concurrent call safety. Confirm IMasternodeVotingContract and IRewardsStore implementations are safe for concurrent RPC dispatch, or set to false until verified.

---

[claude]

Medium — no round-trip test for eth_getTransactionAndReceiptProof with a valid transaction (flagged 4 reviews)

Only the not-found path is tested. The proof format — key RLP encoding, FromProofNodes hash computation, TxRoot/ReceiptRoot derivation — is entirely untested. Given this is custom proof-format code, at least one test with a real block + known transaction should be added to verify the round-trip.

Suggested structure (using TestBlockchain or a minimal in-memory setup):

[Test]
public async Task eth_getTransactionAndReceiptProof_returns_valid_proof_for_known_transaction()
{
    // Arrange: build a block with at least one transaction, set up real receipt finder
    // Act: call eth_getTransactionAndReceiptProof with the known tx hash
    // Assert: result is non-null, TxRoot matches the block's TxRoot, ReceiptRoot matches,
    //         Key == RLP-encode(index), TxProofKeys/Values are non-empty
}

[claude]

Low — header.Hash! null-forgiving operator

header came from FindHeader(blockHash) so its hash should equal blockHash in practice, but the null-forgiving operator hides a potential NullReferenceException. Consistent with the fix applied to block.Hash in this same PR:

Suggested change

	if (!rewardsStore.TryGetEpochRewards(header.Hash!, out Dictionary<string, Dictionary<string, Dictionary<string, string>>>? rewards)
	if (!rewardsStore.TryGetEpochRewards(header.Hash ?? blockHash, out Dictionary<string, Dictionary<string, Dictionary<string, string>>>? rewards)

Using header.Hash ?? blockHash is safe here since the block was found by that hash.