Skip to content

Fix light client merkle proofs #7007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mergify merged 4 commits into from
Feb 18, 2025
Merged

Fix light client merkle proofs #7007

mergify merged 4 commits into from
Feb 18, 2025

Conversation

@michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Feb 17, 2025 •
edited
Loading

Issue Addressed

Fix a regression introduced in this PR:

We were indexing into the MerkleTree with raw generalized indices, which was incorrect and triggering debug_assert failures, as described here:

Proposed Changes

  • Convert generalized_index to the correct leaf index prior to proof generation.
  • Add sanity checks on indices used in BeaconState::generate_proof.
  • Remove debug asserts from MerkleTree::generate_proof in favour of actual errors. This would have caught the bug earlier.
  • Refactor the EF tests so that the merkle validity tests are actually run. They were misconfigured in a way that resulted in them running silently with 0 test cases, and the check_all_files_accessed.py script still had an ignore that covered the test files, so this omission wasn't detected.

Additional Info

This bug only affects LightClientFinality update and LightClientBootstrap, as they are the consumers of finalized root and sync committee merkle proofs respectively. The processing and broadcast of LightClientOptimisticUpdate is unaffected.

The buggy behaviour could be seen on our infra in the form of debug logs:

DEBG Light client invalid finality update error: InvalidLightClientFinalityUpdate, peer: 16Uiu2HAmAgnZHgwbHYVDiahFbT6pzeg5HT28fsYwwmGJD6fxbQQh

I am monitoring to ensure that these errors disappear after roll out.

@michaelsproul michaelsproul added bug Something isn't working light-client electra Required for the Electra/Prague fork v7.0.0 New release c. Q1 2025 labels Feb 17, 2025
@michaelsproul michaelsproul mentioned this pull request Feb 17, 2025
Closed
@michaelsproul michaelsproul added the ready-for-review The code is ready for review label Feb 17, 2025
@michaelsproul
Copy link
Member Author

michaelsproul commented Feb 17, 2025

I've removed the logging for now, but it is running on our infra to help us spot more issues.

There are a lot of fields and subfields, so unless we do something like CompareFields (might be viable?), the output is going to be very long. Even as JSON it's quite unmanagable. E.g.

{
  "attested_header": {
    "beacon": {
      "slot": "3656194",
      "proposer_index": "1535871",
      "parent_root": "0x853c87a6bb14c9bbcfe98f366ea09948294cf209bc951d5310ec9e6dbfef86b3",
      "state_root": "0x906f1ff01ea2556bb6a05f4dfa7f3673f8a4a9d0f3ba0251600f61c756784fac",
      "body_root": "0x0f5cd8fbc7c4df7fd91e8c1b9c6cfa9ad711fc631a2a51d1197fe5e660049ad6"
    },
    "execution": {
      "parent_hash": "0x4113b40798acf7b57ee9391d3174e529fd85aa26610340eca9f63fabbca0bfce",
      "fee_recipient": "0x3b41d9736ed9bfc15a87d8fbb69c616190aed6c6",
      "state_root": "0x12de0a979f894c9cdbe94b85d14d01d24029d76cc196fd82fb385013fc4cd1fb",
      "receipts_root": "0xc02bb1e5c198cbccc21144765d0cef8f4bdec1c6a0d1b0a236f7c989da67d618",
      "logs_bloom": "0x84000000000000400080000000000000000000000400000000080040000000000100000000010000000001800000000400000000000800000040000000040000000000000000000000400000000000400000004000040000401000000000000100000000020000000000000000082800000000080000100000000000000000000000040004000004200000000000000000000000000000000000010000000000000000140000000000000100000004000000000000000002100004000000000040000000000000000100000000000000000000000000a00000000000000062000000000000000000209000000000000000000080008000000000000000100000",
      "prev_randao": "0x7306fb2898c6ccdaf4960d4ddfce66eaa81d9403e1155dd577e0f48b41519ff0",
      "block_number": "3369447",
      "gas_limit": "35964845",
      "gas_used": "652202",
      "timestamp": "1739776728",
      "extra_data": "0xd883010e0d846765746888676f312e32332e33856c696e7578",
      "base_fee_per_gas": "1254913",
      "block_hash": "0x190dfc8f7c4c32431477a771435bd3b0ed5c6df13fdab726d4ab900679ce7873",
      "transactions_root": "0xeba10054b5b2c487f259aa214e5e263d05a021eab955bc1b1529d5767dd1e99a",
      "withdrawals_root": "0xb3a26f1b2b68f43c04dea5e09172b1d9c0ae25dc13edad4d3d230149d8340bde",
      "blob_gas_used": "131072",
      "excess_blob_gas": "0"
    },
    "execution_branch": [
      "0x639ea3536f8ac6fc831cf4c3629f1f2a59e6baff009553dedef9bc0adcd40189",
      "0xd8857450a5aa5ef7f46be1ca6e14dfd20b698a047f30364e67c4c9dbd1dcd5f9",
      "0xdb56114e00fdd4c1f85c892bf35ac9a89289aaecb1ebd0a96cde606a748b5d71",
      "0x410b66c5afae89b1df7a65350ed742bf05942f75d6b663abfd295c626c1ec7f4"
    ]
  },
  "finalized_header": {
    "beacon": {
      "slot": "3656128",
      "proposer_index": "1867558",
      "parent_root": "0xee531cf5c2e68f032233aec782b73ac5bfd4bb289402868947a007aad8144d8d",
      "state_root": "0x83141bc902bcdd4557037df2b2b7c0bc79debb27d941a1d16f31218ea5a5dfcb",
      "body_root": "0xdeefa80e9b552e677cdb286f2813bb7e4e62118cec603b0dd3b932889adea866"
    },
    "execution": {
      "parent_hash": "0xd8cbcae880431bb6128ac8ef16a0a67c0e15c8b61e3fba3df902d3a5b38a0aa5",
      "fee_recipient": "0xb6c402298fcb88039bbfde70f5ace791f18cfac8",
      "state_root": "0xaf3cc69f7d9258acee618c36c903a1c8bd5cf6e22cb1c40a7d7c490fb763555d",
      "receipts_root": "0x024a1cd65cb6ef3d50a22ea42c4dbdef586a5851e4df91cb4a5443b8c988ad7d",
      "logs_bloom": "0x263320c08622632074924040000c0042344e0026b7800747e4ed407c07d0004a0173618728819201a0c01406c064000f5102801a0818069420440880803564814022206903255106c802c02a2ba0c54030008440888fa43200900000010648048841b5640241448780308a8f801a0a95b5429081013c511200da8cb2e8007c0481142d0d146c248a280404100084031600033060809040666e418280044c4034b289023424888647d52809402a30071061901430212210c80c28521102c130404110018288610180850580001103082180500c20870fb49880570cb2102c670d801d012c14400800248040342010e525242090b820e048540160006430103149",
      "prev_randao": "0x504aef912cc0fe4258063b34d3c5bba9992f86a1bfac9ad5ba04a61a5435fe10",
      "block_number": "3369387",
      "gas_limit": "35894605",
      "gas_used": "29816669",
      "timestamp": "1739775936",
      "extra_data": "0xd883010e0b846765746888676f312e32332e31856c696e7578",
      "base_fee_per_gas": "593270",
      "block_hash": "0x391a92b9caacccb4d2576346d945a43723b5c2a4450e55d86c1a9565acb76f1c",
      "transactions_root": "0xf64a6d39ebef81fe1bfdab817045ce711ab3c4b81b312d528dd3dc3efcc7878c",
      "withdrawals_root": "0x0bd45d0ed89944f58a97bb9594eae83be449f5f5aa73901e3e873903b6fdd73b",
      "blob_gas_used": "0",
      "excess_blob_gas": "0"
    },
    "execution_branch": [
      "0xd51aceed9aff38ded132761162c49a77523ae7c1dd74fa34f9acdcce9b172fc3",
      "0xb46f0c01805fe212e15907981b757e6c496b0cb06664224655613dcec82505bb",
      "0xdb56114e00fdd4c1f85c892bf35ac9a89289aaecb1ebd0a96cde606a748b5d71",
      "0x12fb543c0b5f8c609fa24f368dded12589f1ab387378ea8fbe40c46ae9969179"
    ]
  },
  "finality_branch": [
    "0x4ebe010000000000000000000000000000000000000000000000000000000000",
    "0x84aac78f6d6e4173f7aa524089ebe1322b50fa51332cfc4d61ab427e9e324d3c",
    "0xddff8ee532aa33a03e7839404940622daf61580f110efcb6459a536f8aee00e1",
    "0xc2c86a8bedbef9c6c9036c3a7b54587f25d9be732a44ffba66fdd49716679782",
    "0xd38854d340308969d869f85211f0918be250cdac61e43c272cf9f07d1172802e",
    "0x8192c89a42969df1a43afbc4032391b9e1d4771752313f742159db5910c86c1f"
  ],
  "sync_aggregate": {
    "sync_committee_bits": "0xf7ff7ffeffffdbfffdffeefafffff7dffffff6f7ffffbfeffbbfffffff9ffffdfffffeffdfbfffebffffffffffffffffeffffffefeefffffffffffffffffffe5",
    "sync_committee_signature": "0xa484f9534bf0014553dde697df805cb79f8b1269bd7bc6cab2f704d7fbb0702a22fde012c8eee224b8508a02c5bac0e7117feab34ad0f552e50d7849f15d30021b6b7ec2dfaa481bca7d6aa491eab1647aaf959d42748f19029ba3397e8fb046"
  },
  "signature_slot": "3656195"
}

eserilev
eserilev approved these changes Feb 17, 2025
View reviewed changes
Copy link
Member

@eserilev eserilev left a comment

Choose a reason for hiding this comment

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

thanks for catching this, LGTM

dapplion
dapplion reviewed Feb 17, 2025
View reviewed changes
consensus/types/src/beacon_state.rs
} else {
light_client_update::CURRENT_SYNC_COMMITTEE_INDEX
};
let field_index = field_gindex.safe_sub(self.num_fields_pow2())?;
Copy link
Collaborator

@dapplion dapplion Feb 17, 2025

Choose a reason for hiding this comment

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

Should we make the GIndex a type to prevent these issues in the future?

Copy link
Member Author

@michaelsproul michaelsproul Feb 18, 2025

Choose a reason for hiding this comment

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

Yeah I think that sounds like a good idea. Maybe in a separate PR, as I'd like to keep this small and minimal for v7.0.0-beta.1 which we should release ASAP

Copy link
Member Author

@michaelsproul michaelsproul Feb 18, 2025

Choose a reason for hiding this comment

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

For now I think the EF tests and index checks are good insurance against future mistakes

@michaelsproul
Copy link
Member Author

michaelsproul commented Feb 18, 2025

Update after running overnight:

  • Very few occurrences of invalid finality updates floating around. There were 36 total, and the 3 I looked into were all Lodestar sending really stale updates. I've opened this issue on their repo: Stale LightClientFinalityUpdates sent during sync ChainSafe/lodestar#7476. This suggests that a staleness check would be an easy way to get better insights about the types of invalid LC updates. This was already being considered for different reasons here, which makes me feel like it is the right approach: Light client optimistic updates are requeued too eagerly #6994.
  • The lack of invalid updates coming from Lighthouse peers (our un-updated nodes running v7.0.0-beta.0) is explained by the fact that we don't broadcast light client updates yet. So the impact of this bug for now is mostly that we don't forward updates that we should forward. It's possible that an attacker could forward junk updates that we would (wrongly) verify with v7.0.0-beta.0, so we should definitely fix this before v7.0.0 on mainnet, but whether we want to do v7.0.0-beta.1 will depend on the need for other fixes (e.g. builder API).
  • No occurrences of invalid optimistic updates overnight, although there were a few yesterday prior to restart. I'll continue monitoring.

@michaelsproul michaelsproul added the ready-for-merge This PR is ready to merge. label Feb 18, 2025
@michaelsproul michaelsproul mentioned this pull request Feb 18, 2025
Open
@mergify mergify bot merged commit ff739d5 into sigp:release-v7.0.0-beta.0 Feb 18, 2025
31 checks passed
@michaelsproul michaelsproul deleted the fix-light-client-merkle-proofs branch February 18, 2025 01:07
@syjn99 syjn99 mentioned this pull request Feb 18, 2025
Merged
@ncitron ncitron mentioned this pull request Feb 20, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dapplion dapplion dapplion left review comments

@eserilev eserilev eserilev approved these changes

Assignees

No one assigned

Labels

bug Something isn't working electra Required for the Electra/Prague fork light-client ready-for-merge This PR is ready to merge. ready-for-review The code is ready for review v7.0.0 New release c. Q1 2025

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@michaelsproul @eserilev @dapplion