Skip to content

Comments

[types] Add committed_hash() to Transaction and remove CryptoHash trait#18590

Merged
ibalajiarun merged 5 commits intomainfrom
ibalajiarun/berlin
Feb 10, 2026
Merged

[types] Add committed_hash() to Transaction and remove CryptoHash trait#18590
ibalajiarun merged 5 commits intomainfrom
ibalajiarun/berlin

Conversation

@ibalajiarun
Copy link
Contributor

@ibalajiarun ibalajiarun commented Feb 4, 2026 •
edited
Loading

Summary

This PR introduces a committed_hash() method on Transaction and removes the CryptoHash trait implementation in production to enforce its usage at compile-time.

Changes

  1. Add committed_hash() method to Transaction

    • For UserTransaction, delegates to SignedTransaction::committed_hash() which uses caching
    • For other variants, computes hash via a unified helper function

  2. Remove BCSCryptoHash derive from Transaction (production only)

    • Prevents accidental use of .hash() method in production code
    • Compile-time enforcement that committed_hash() is the only way to get a Transaction's hash

  3. Conditional BCSCryptoHash derive for testing

    • In test/fuzzing mode, Transaction derives BCSCryptoHash
    • Tests verify committed_hash() matches CryptoHash::hash()
    • If CryptoHash behavior changes, tests will fail, alerting us to update our implementation

  4. Update all call sites (25+ locations across the codebase)

    • Storage layer, execution layer, API layer, and tests

Architecture

Transaction::committed_hash()
  |-- UserTransaction(txn) -> txn.committed_hash()  [uses SignedTransaction's cache]
  |                            `-- compute_transaction_hash() on cache miss
  `-- Other variants -> compute_transaction_hash()

Benefits

  • Compile-time safety: Calling .hash() on Transaction is a compile error in production
  • Single source of truth: compute_transaction_hash() is the only place that knows how to hash
  • Caching preserved: UserTransaction still benefits from SignedTransaction's OnceCell cache
  • Test coverage: Tests verify our implementation matches CryptoHash trait behavior
  • Future flexibility: This design allows the hash to be derived differently if necessary. One such use case is Encrypted Transactions, where the committed hash may need to be computed differently from the standard BCS serialization approach.

Test plan

  • cargo check passes for all affected crates
  • Unit tests verify committed_hash() matches CryptoHash::hash()
  • CI tests

Generated with Claude Code

@ibalajiarun ibalajiarun force-pushed the ibalajiarun/berlin branch 2 times, most recently from cff8abf to e651ed1 Compare February 6, 2026 20:34
@ibalajiarun ibalajiarun added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Feb 6, 2026
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
JoshLind
JoshLind approved these changes Feb 10, 2026
View reviewed changes
Copy link
Contributor

@JoshLind JoshLind left a comment

Choose a reason for hiding this comment

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

Thanks @ibalajiarun! 😄

@ibalajiarun ibalajiarun enabled auto-merge (squash) February 10, 2026 15:16
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
ibalajiarun and others added 5 commits February 10, 2026 10:08
@ibalajiarun @claude
[types] Add committed_hash() method to Transaction
aaca9ba 
Introduce a committed_hash() method on Transaction that provides the
hash used when a transaction is committed to the ledger. For
UserTransaction, this leverages the existing cache in SignedTransaction.

Update all call sites to use committed_hash() instead of hash() directly,
providing clearer semantics and preparing for future enforcement.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@ibalajiarun @claude
[types] Remove CryptoHash trait from Transaction
940cbe9 
Remove BCSCryptoHash derive from Transaction enum to prevent accidental
use of .hash() method. All hash computation now goes through the unified
compute_transaction_hash() helper function.

This enforces at compile-time that committed_hash() is the only way to
get a Transaction's hash, preventing mistakes where .hash() was called
directly.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@ibalajiarun @claude
[types] Fix remaining .hash() calls in proof tests
71deeb8 
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@ibalajiarun @claude
[types] Add unit tests for Transaction::committed_hash()
fa9d192 
Add tests to verify:
1. committed_hash() returns the same value as CryptoHash::hash()
2. SignedTransaction caches the committed_hash correctly

Uses conditional BCSCryptoHash derive (test/fuzzing only) so tests can
compare committed_hash() against the real CryptoHash implementation.
This ensures our manual implementation stays in sync with the trait.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@ibalajiarun @claude
[types] Fix lint: unused import and formatting
de17cb8 
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions

This comment has been minimized.

Sign in to view
@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2026

✅ Forge suite compat success on e3bc294252919be7e11a4bddba7f0c6e2a79f541 ==> de17cb81219010aa24eeec1fbf75d645ccc87245

Forge report malformed: Expecting ',' delimiter: line 1 column 6 (char 5)
'[2026-02-10T18:44:19Z INFO  aptos_forge::report] Test Ok\n{\n  "metrics": [\n    {\n      "test_name": "compatibility::simple-validator-upgrade::liveness-check",\n      "metric": "submitted_txn",\n      "value": 452420.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::liveness-check",\n      "metric": "expired_txn",\n      "value": 0.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::liveness-check",\n      "metric": "avg_tps",\n      "value": 13801.848537621583\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::liveness-check",\n      "metric": "avg_latency",\n      "value": 2519.2015406038636\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::liveness-check",\n      "metric": "p50_latency",\n      "value": 2700.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::liveness-check",\n      "metric": "p90_latency",\n      "value": 3000.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::liveness-check",\n      "metric": "p99_latency",\n      "value": 3600.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::single-validator-upgrade",\n      "metric": "submitted_txn",\n      "value": 207760.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::single-validator-upgrade",\n      "metric": "expired_txn",\n      "value": 0.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::single-validator-upgrade",\n      "metric": "avg_tps",\n      "value": 6089.095084112674\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::single-validator-upgrade",\n      "metric": "avg_latency",\n      "value": 5552.941615325375\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::single-validator-upgrade",\n      "metric": "p50_latency",\n      "value": 6000.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::single-validator-upgrade",\n      "metric": "p90_latency",\n      "value": 6400.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::single-validator-upgrade",\n      "metric": "p99_latency",\n      "value": 6500.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::half-validator-upgrade",\n      "metric": "submitted_txn",\n      "value": 210240.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::half-validator-upgrade",\n      "metric": "expired_txn",\n      "value": 0.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::half-validator-upgrade",\n      "metric": "avg_tps",\n      "value": 6043.319293810853\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::half-validator-upgrade",\n      "metric": "avg_latency",\n      "value": 5628.248715753424\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::half-validator-upgrade",\n      "metric": "p50_latency",\n      "value": 6100.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::half-validator-upgrade",\n      "metric": "p90_latency",\n      "value": 6600.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::half-validator-upgrade",\n      "metric": "p99_latency",\n      "value": 6800.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::rest-validator-upgrade",\n      "metric": "submitted_txn",\n      "value": 346680.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::rest-validator-upgrade",\n      "metric": "expired_txn",\n      "value": 0.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::rest-validator-upgrade",\n      "metric": "avg_tps",\n      "value": 10495.659688278809\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::rest-validator-upgrade",\n      "metric": "avg_latency",\n      "value": 3111.443887735087\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::rest-validator-upgrade",\n      "metric": "p50_latency",\n      "value": 3100.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::rest-validator-upgrade",\n      "metric": "p90_latency",\n      "value": 4100.0\n    },\n    {\n      "test_name": "compatibility::simple-validator-upgrade::rest-validator-upgrade",\n      "metric": "p99_latency",\n      "value": 4800.0\n    }\n  ],\n  "text": "Compatibility test results for e3bc294252919be7e11a4bddba7f0c6e2a79f541 ==> de17cb81219010aa24eeec1fbf75d645ccc87245 (PR)\\n1. Check liveness of validators at old version: e3bc294252919be7e11a4bddba7f0c6e2a79f541\\ncompatibility::simple-validator-upgrade::liveness-check : committed: 13801.85 txn/s, latency: 2519.20 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3600 ms), latency samples: 452420\\n2. Upgrading first Validator to new version: de17cb81219010aa24eeec1fbf75d645ccc87245\\ncompatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6089.10 txn/s, latency: 5552.94 ms, (p50: 6000 ms, p70: 6300, p90: 6400 ms, p99: 6500 ms), latency samples: 207760\\n3. Upgrading rest of first batch to new version: de17cb81219010aa24eeec1fbf75d645ccc87245\\ncompatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6043.32 txn/s, latency: 5628.25 ms, (p50: 6100 ms, p70: 6400, p90: 6600 ms, p99: 6800 ms), latency samples: 210240\\n4. upgrading second batch to new version: de17cb81219010aa24eeec1fbf75d645ccc87245\\ncompatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10495.66 txn/s, latency: 3111.44 ms, (p50: 3100 ms, p70: 3500, p90: 4100 ms, p99: 4800 ms), latency samples: 346680\\n5. check swarm health\\nCompatibility test for e3bc294252919be7e11a4bddba7f0c6e2a79f541 ==> de17cb81219010aa24eeec1fbf75d645ccc87245 passed\\nTest Ok"\n}'
Trailing Log Lines:
2. Upgrading first Validator to new version: de17cb81219010aa24eeec1fbf75d645ccc87245
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6089.10 txn/s, latency: 5552.94 ms, (p50: 6000 ms, p70: 6300, p90: 6400 ms, p99: 6500 ms), latency samples: 207760
3. Upgrading rest of first batch to new version: de17cb81219010aa24eeec1fbf75d645ccc87245
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6043.32 txn/s, latency: 5628.25 ms, (p50: 6100 ms, p70: 6400, p90: 6600 ms, p99: 6800 ms), latency samples: 210240
4. upgrading second batch to new version: de17cb81219010aa24eeec1fbf75d645ccc87245
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 10495.66 txn/s, latency: 3111.44 ms, (p50: 3100 ms, p70: 3500, p90: 4100 ms, p99: 4800 ms), latency samples: 346680
5. check swarm health
Compatibility test for e3bc294252919be7e11a4bddba7f0c6e2a79f541 ==> de17cb81219010aa24eeec1fbf75d645ccc87245 passed
Test Ok

=== BEGIN JUNIT ===
<?xml version="1.0" encoding="UTF-8"?>
<testsuites name="forge" tests="1" failures="0" errors="0" uuid="8c78a5b0-b1ca-45c3-a738-970599f19963">
    <testsuite name="compat" tests="1" disabled="0" errors="0" failures="0">
        <testcase name="compatibility::simple-validator-upgrade">
        </testcase>
    </testsuite>
</testsuites>
=== END JUNIT ===
[2026-02-10T18:44:19Z INFO  aptos_forge::backend::k8s::cluster_helper] Deleting namespace forge-compat-pr-18590: Some(NamespaceStatus { conditions: None, phase: Some("Terminating") })
[2026-02-10T18:44:19Z INFO  aptos_forge::backend::k8s::cluster_helper] aptos-node resources for Forge removed in namespace: forge-compat-pr-18590

test result: ok. 1 passed; 0 soft failed; 0 hard failed; 0 filtered out

Debugging output:
NAME                                         READY   STATUS      RESTARTS   AGE
aptos-node-0-validator-0                     1/1     Running     0          6m36s
aptos-node-1-validator-0                     1/1     Running     0          8m14s
aptos-node-2-validator-0                     1/1     Running     0          4m52s
aptos-node-3-validator-0                     1/1     Running     0          3m51s
forge-testnet-deployer-ncmx8                 0/1     Completed   0          12m
genesis-aptos-genesis-eforge9c021145-bcmbv   0/1     Completed   0          11m

@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2026

✅ Forge suite realistic_env_max_load success on de17cb81219010aa24eeec1fbf75d645ccc87245

two traffics test: inner traffic : committed: 13394.88 txn/s, submitted: 13395.05 txn/s, expired: 0.16 txn/s, latency: 2813.67 ms, (p50: 2700 ms, p70: 3000, p90: 3300 ms, p99: 4100 ms), latency samples: 4988200
two traffics test : committed: 100.01 txn/s, latency: 788.13 ms, (p50: 700 ms, p70: 800, p90: 1000 ms, p99: 1500 ms), latency samples: 1620
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.504, avg: 2.200", "ConsensusProposalToOrdered: max: 0.168, avg: 0.166", "ConsensusOrderedToCommit: max: 0.050, avg: 0.044", "ConsensusProposalToCommit: max: 0.217, avg: 0.210"]
Max non-epoch-change gap was: 1 rounds at version 40407 (avg 0.00) [limit 4], 1.16s no progress at version 40407 (avg 0.07s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.29s no progress at version 2401871 (avg 0.29s) [limit 16].
Test Ok

@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2026

✅ Forge suite framework_upgrade success on e3bc294252919be7e11a4bddba7f0c6e2a79f541 ==> de17cb81219010aa24eeec1fbf75d645ccc87245

Compatibility test results for e3bc294252919be7e11a4bddba7f0c6e2a79f541 ==> de17cb81219010aa24eeec1fbf75d645ccc87245 (PR)
Upgrade the nodes to version: de17cb81219010aa24eeec1fbf75d645ccc87245
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1503.06 txn/s, submitted: 1508.15 txn/s, failed submission: 5.09 txn/s, expired: 5.09 txn/s, latency: 1969.72 ms, (p50: 1200 ms, p70: 1500, p90: 2000 ms, p99: 12000 ms), latency samples: 135741
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 2403.65 txn/s, submitted: 2411.81 txn/s, failed submission: 8.16 txn/s, expired: 8.16 txn/s, latency: 1202.08 ms, (p50: 1200 ms, p70: 1200, p90: 1500 ms, p99: 2300 ms), latency samples: 218061
5. check swarm health
Compatibility test for e3bc294252919be7e11a4bddba7f0c6e2a79f541 ==> de17cb81219010aa24eeec1fbf75d645ccc87245 passed
Upgrade the remaining nodes to version: de17cb81219010aa24eeec1fbf75d645ccc87245
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1532.99 txn/s, submitted: 1537.80 txn/s, failed submission: 4.81 txn/s, expired: 4.81 txn/s, latency: 1938.09 ms, (p50: 1200 ms, p70: 1500, p90: 2100 ms, p99: 12900 ms), latency samples: 140221
Test Ok

@ibalajiarun ibalajiarun merged commit f3d060a into main Feb 10, 2026
61 of 62 checks passed
@ibalajiarun ibalajiarun deleted the ibalajiarun/berlin branch February 10, 2026 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@grao1991 grao1991 grao1991 approved these changes

@JoshLind JoshLind JoshLind approved these changes

@lightmark lightmark Awaiting requested review from lightmark lightmark is a code owner

@wrwg wrwg Awaiting requested review from wrwg

@zekun000 zekun000 Awaiting requested review from zekun000

@vgao1996 vgao1996 Awaiting requested review from vgao1996

@georgemitenkov georgemitenkov Awaiting requested review from georgemitenkov

@banool banool Awaiting requested review from banool banool is a code owner

@gregnazario gregnazario Awaiting requested review from gregnazario gregnazario is a code owner

@0xmaayan 0xmaayan Awaiting requested review from 0xmaayan 0xmaayan is a code owner

@danielxiangzl danielxiangzl Awaiting requested review from danielxiangzl

Assignees

No one assigned

Labels

CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ibalajiarun @grao1991 @JoshLind