Remove legacy Ethereum OPP finalizer dependency#526
Conversation
heifner
left a comment
There was a problem hiding this comment.
Automated pre-merge review (code-review, xhigh effort). No correctness bugs found — the finalizeEpoch removal, emitOutboundEnvelope(uint32) retype, ABI fixture, and new tests are internally consistent, and no external caller breaks. 4 low-severity, non-blocking notes inline (test-coverage precision, conventions, one doc/dead-code consistency nit).
| ethereum_contract_tx_fn<fc::variant> finalize_epoch; | ||
| /// Recovery-only write matching `emitOutboundEnvelope(uint32)` on the | ||
| /// Ethereum outpost. Normal operation emits during inbound consensus. | ||
| ethereum_contract_tx_fn<fc::variant, uint32_t> emit_outbound_envelope; |
There was a problem hiding this comment.
Test coverage — recovery emitOutboundEnvelope(uint32) is never invoked or behaviorally tested
This retype (emitOutboundEnvelope() → emitOutboundEnvelope(uint32)) is the PR's core code change, but emit_outbound_envelope has no in-tree caller and the new tests only assert its ABI-encoding shape and that the std::function is bound (BOOST_CHECK(client->emit_outbound_envelope)). Nothing verifies that invoking it with a uint32 epoch actually produces correct recovery call data.
Per wire-sysio/CLAUDE.md: "Tests should verify behavior, not just that code compiles." Consider driving the recovery wrapper end-to-end against a mocked tx sink and asserting the epoch lands in the encoded args — or, if it is genuinely operator-tooling-only, a comment pointing at the out-of-tree caller. Non-blocking.
| set_response(encode_latest_outbound_result(7, serialize_envelope(8))); | ||
| BOOST_CHECK(outpost.read_inbound_envelope(7, fc::seconds(1)).empty()); | ||
|
|
||
| std::vector<char> oversized(sysio::OPP_MAX_ENVELOPE_BYTES + 1, char(0x01)); |
There was a problem hiding this comment.
Test coverage — this oversized case exits via the RPC hex cap, not the byte cap it appears to target
OPP_MAX_ENVELOPE_BYTES + 1 (65537 B) encodes to a 131330-hex-char ABI result, which exceeds MAX_LATEST_OUTBOUND_RPC_HEX_CHARS (131266) and returns {} at outpost_ethereum_client.cpp:144 before decoding — so the raw.size() > OPP_MAX_ENVELOPE_BYTES guard at line 212 is never reached (it is in fact unreachable given the earlier string caps). The assertion passes, but for the wrong reason.
If the intent is to cover the byte-level cap, this input can't reach it; if the intent is just "oversized → empty", a one-line comment noting it exercises the RPC-length cap would avoid a false read of the coverage. Non-blocking.
| std::string encode_latest_outbound_result(uint32_t epoch, const std::vector<char>& data) { | ||
| auto data_hex = data.empty() ? std::string{} : fc::to_hex(data.data(), data.size()); | ||
| data_hex.append((64 - (data_hex.size() % 64)) % 64, '0'); | ||
| return "0x" + abi_word(epoch) + abi_word(64) + abi_word(data.size()) + data_hex; |
There was a problem hiding this comment.
Conventions — magic literals in the new ABI helpers (wire-sysio/CLAUDE.md Invariant #2: "Every string or numeric value that isn't a trivial array index / loop bound lives behind a named constexpr", applied to every change in the repo)
abi_word(64)here — the64is the ABI tail offset (the two 32-byte head words of the(uint32, bytes)return). If that tuple shape ever changes, this silently produces a wrong offset.std::setw(64)on line 169 — the 32-byte-word hex width.- Selector literal
"a3ad9cc3", the72u/8usizes, and the raw1chain-kind arg passed to theoutpost_ethereum_client(...)ctor.
The production decoder already names these (EVM_ABI_WORD_BYTES, HEX_CHARS_PER_BYTE, …); suggest reusing/adding named constants (ABI_WORD_HEX_WIDTH, ABI_HEAD_BYTES, an expected-selector constant). Non-blocking.
| * `emit_outbound_envelope` so the outpost emits any queued outgoing ones — | ||
| * the return value is the signature of the second call (the one that signals | ||
| * "work done for this epoch"). | ||
| * `deliver_outbound_envelope` stages chunks through `epoch_in`, then sends a |
There was a problem hiding this comment.
Consistency — this doc now says the inline emit replaces the separate call, but the plugin still binds it
This rewrite (and the outpost_solana_client.cpp "No separate emit_outbound_envelope" comment) states the terminal epoch_in emits the outbound envelope inline. However outpost_solana_client_plugin.hpp:324 still binds solana_program_tx_fn<std::string, uint32_t> emit_outbound_envelope, which a grep shows is never invoked. The prose and the retained binding disagree.
That binding is in a file this PR doesn't touch, so it's pre-existing — but since this doc change is what surfaces the inconsistency, worth either dropping the dead binding or noting why it's retained (recovery parity with the ETH side?). Non-blocking.
heifner
left a comment
There was a problem hiding this comment.
Approving. ✅
The core change is sound: removing finalizeEpoch + retyping emitOutboundEnvelope(uint32) is internally consistent across the plugin header, the ABI fixture (emitOutboundEnvelope(uint32), getLatestOutboundEnvelope → (uint32 epoch_, bytes data_), finalizeEpoch gone), and the tests, and no external caller breaks from the finalize_epoch removal.
All four of my earlier review notes are addressed in e347796a, with no regressions in the delta:
- Recovery
emitOutboundEnvelope(uint32)— newemit_outbound_envelope_recovery_wrapper_forwards_wire_epochtest asserts the wrapper forwards the epoch and produces the exact call data; header doc clarifies it's a recovery-tooling-only surface. oversizedcase — now documents that it verifies the pre-decode RPC hex-length boundary (not the decoded-byte cap), and the var/const are renamedrpc_length_oversized*.- Magic literals — replaced with a named-constant block (
evm_abi_word_hex_chars,latest_outbound_data_offset_bytes,emit_outbound_envelope_selector,test_wire_epoch, …); all preserve the original values. - Solana
emit_outbound_envelopebinding — documented at the declaration, the init site, and the.cppcomment as an explicit recovery/admin escape hatch, reconciling the doc with the retained code.
Verified the new constants are value-preserving, encode_latest_outbound_result is equivalent, and the recovery test's lambda matches the std::function<RT(Args&...)> non-const-ref convention. No remaining findings.
heifner
left a comment
There was a problem hiding this comment.
Approve. Confirmed the removed finalize_epoch wrapper had no other in-tree references, the fixture matches the post-#160 ABI surface (and fixes the previously stale zero-input emitOutboundEnvelope entry), and the new latest-slot test drives the real read path — get_contract caches per address, so the stubbed typed client is the same instance the outpost uses.
One suggestion for the description or release notes: an older nodeop binary given the new Ethereum ABI artifact fails at client construction (get_abi("finalizeEpoch") throws), so operator binaries need to upgrade before (or together with) switching their configured ABI artifacts. Merge order among the three companion PRs is otherwise free.
Summary
finalizeEpochABI wrapper and constructor lookupemitOutboundEnvelope(uint32)recovery wrapper and current ABI fixture with EthereumContext
These changes are review follow-ups from Wire-Network/wire-ethereum#155, covering work that was intentionally separated from that PR.
Why
The relay reads
getLatestOutboundEnvelope; it never needs legacy Ethereum finalizer storage. Requiring the removed ABI function would prevent client construction after the Ethereum cleanup.Validation
test_outpost_ethereum_client_pluginandtest_outpost_solana_client_plugintargetsdfb763c84b; the review follow-up is test and documentation onlyCompanion changes