Skip to content

ci: Fix host function audit script - #293

Closed
sappenin wants to merge 9 commits into
mainfrom
df/fix-host-function-audit
Closed

sappenin wants to merge 9 commits into
mainfrom
df/fix-host-function-audit

Conversation

@sappenin

@sappenin sappenin commented Sep 4, 2026 •

Copy link
Copy Markdown
Collaborator

The se/supported branch has been updated to use Wasmi 2.0 with Rust bindings. This PR updates compareHostFunctions.js to check the new xrpld host binding definitions against this project’s host bindings trait.

The `se/supported` branch has no been updated to use Wasmi 2.0 with Rust bindings. This PR updates `compareHostFunctions.js` to check the new xrpld host binding definitions against this project’s host bindings trait.

Signed-off-by: David Fuelling <sappenin@gmail.com>
@sappenin sappenin self-assigned this Sep 4, 2026
@sappenin
sappenin requested a review from mvadari September 4, 2026 17:17

@ripple-code-reviewer ripple-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The rewrite of tools/compareHostFunctions.js replaces the old C++-based ABI comparison with a Rust-to-Rust comparison (host_functions! declarations vs register.rs registrations), which matches the description in AGENTS.md. The parsing logic itself looks internally consistent (duplicate detection, name-based cross-referencing, wasm-value-level type comparison), but two things stand out: the new parsing functions contain real regex/data-transformation logic with no accompanying test file, and parseAbiDeclarations lacks the kind of self-consistency check (parsed-count vs raw-arm-count) that parseAbiRegistrations added for itself, so a return-type shape it can't parse (e.g. a nested generic in HostResult<...>) would silently drop that function rather than failing loudly.

@sappenin sappenin added this to the v1.0.0 milestone Sep 4, 2026
@sappenin sappenin mentioned this pull request Sep 4, 2026
2 tasks done
@sappenin sappenin changed the title Fix host function audit script ci: Fix host function audit script Sep 4, 2026

@ripple-code-reviewer ripple-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The rewrite replaces the old C++-based parser with a Rust-source parser (declarations in lib.rs, registrations in register.rs) and correlates them by PascalCase variant name, which matches the updated AGENTS.md description. Two spots in the new parsing logic are worth a second look: the macro-block boundary detection and the mandatory-return-type assumption in the declaration regex — both are places where a legitimate but slightly different upstream formatting could cause the audit to misparse or spuriously fail, which is exactly the kind of thing this MR is trying to fix.

@ripple-code-reviewer ripple-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This MR replaces the old C++-based host-function comparison in tools/compareHostFunctions.js with a Rust-ABI-based comparison (parsing crates/xrpl-host-functions/src/lib.rs declarations and crates/xrpl-wasm-vm/src/register.rs registrations), and updates AGENTS.md to document the new correlation strategy. The parsing logic is defensive throughout: unmatched blocks, duplicate variants, and arm/closure-count mismatches all exit(1) with a clear error rather than silently producing wrong results, and the design assumptions (e.g. that register.rs closures use flat i32/i64 params, not u32/u64/usize) are explicitly documented in code comments, which addresses the main correctness concern I initially had about type-normalization asymmetry between the two sides of the comparison. AGENTS.md's description of the new script behavior matches the implementation (variant correlation via PascalCase of the fn name, params/return sourced only from register.rs, wasm-value-level comparison). I did not find any changed lines with a clear, high-confidence bug, security issue, or logic error; the remaining risk (e.g. regexes not matching if rippled reshapes its macro/match-arm syntax) is inherent to this kind of source-scraping script and is already called out as an accepted trade-off in AGENTS.md ('a genuine set difference... is a real finding to report, not something to paper over in the script').

@ripple-code-reviewer ripple-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a solid rewrite of the host-function audit script to match the new Rust-based rippled ABI (host_functions! declarations + register.rs registrations), and the AGENTS.md update accurately documents the new comparison approach (PascalCase variant correlation, wasm-level type comparison, params/return sourced only from register.rs). I traced through the new regexes (declRegex, closureRegex, armRegex), the block-boundary detection (indexOf("\n}")), the duplicate-registration/duplicate-wasm-name checks, and the paramTranslation table, and didn't find a clear correctness bug introduced by this diff — the guard rails (armCount vs registrations.size, duplicate variant/wasm_name detection, set-equality checks with clear diff reporting) look intentional and defensive. The main residual risk is inherent regex fragility against upstream's exact source formatting (e.g., exact literal HOST_MODULE, op.wasm_name(), prefix, Caller<'_, VmState<'_>> spelling, requiring an explicit -> HostResult<...> on every declaration), but the AGENTS.md text explicitly calls this out as expected/acceptable ('breaks whenever upstream reshapes those two files'), so I'm not flagging it as a bug rather than a known, documented tradeoff. No changed lines exhibit security, resource-leak, or off-by-one issues that I could confirm with confidence.

@ripple-code-reviewer ripple-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The rewrite of compareHostFunctions.js to parse rippled's new Rust-based host ABI (declarations + registrations) instead of the old C++ files looks solid overall, with good defensive checks (duplicate detection, arm-count validation for registrations). One gap: the declaration parser has no equivalent completeness check to the registration parser's armCount validation, so a declaration that doesn't match the regex is silently dropped rather than flagged as a parser bug.

@ripple-code-reviewer ripple-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This MR rewrites tools/compareHostFunctions.js to parse rippled's new Rust-based host ABI (a host_functions! macro block plus a register.rs match arm) instead of the old C++ files, and updates AGENTS.md to document the new comparison model. The rewrite is well-guarded overall: it cross-checks declared vs. registered variant counts and exits loudly rather than silently dropping data in most paths. Two spots are worth a second look before relying on this against the live upstream file, both concerning how declarations are located/parsed inside the host_functions! block.

@ripple-code-reviewer ripple-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a solid, self-contained refactor of the host-function audit script to target the new Rust-based rippled ABI (declaration macro in xrpl-host-functions + registration match-arms in xrpl-wasm-vm) instead of the old C++ WasmVM.cpp/HostFuncWrapper.h files. Variable renames (cppHostFunctions -> rippledHostFunctions, cppHit -> rippledHit, etc.) are consistently applied throughout the diff, the new regexes correctly destructure their capture groups, and duplicate/count-mismatch conditions fail loudly with process.exit(1) rather than silently producing wrong results, which matches the AGENTS.md note that a genuine ABI drift should surface as a real finding. The AGENTS.md documentation update accurately describes the new two-file correlation strategy (declaration attribute for name, registration closure for wire signature). I did not find any correctness or security issues I'm confident about in the added code.

@ripple-code-reviewer ripple-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This MR replaces the old C++-based host function audit with a Rust-based one, correlating rippled's host_functions! declaration macro against the register.rs match arms via regex, then comparing against this project's Rust host bindings. The rewrite is well-documented (AGENTS.md update explains the new correlation strategy and its inherent fragility against upstream changes). The main gaps found are a comment-stripping asymmetry between the two new parser functions that could cause spurious CI failures, and no test coverage for the new regex-based parsing logic despite it now carrying non-trivial business logic.

@ripple-code-reviewer ripple-code-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Duplicate declarations must abort - see inline.

`${ABI_DECLARATION_FILE}: duplicate host function declaration ${rustName}`,
)
process.exit(1)
process.exit(1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Duplicate declarations overwrite silently instead of aborting. Add process.exit(1):

    if (declarations.has(variant)) {
      console.error(
        `${ABI_DECLARATION_FILE}: duplicate host function declaration ${rustName}`,
      )
      process.exit(1)
    }
Suggested change
process.exit(1)
process.exit(1)
}

@sappenin

Copy link
Copy Markdown
Collaborator Author

After thinking about this PR a bit, I've decided to close it in-favor of #316, which instead generates the HostBindings trait directly from the rippled branch that this project pins to.

@sappenin sappenin closed this Sep 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants