Skip to content
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.

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

Download checkpoint blobs during checkpoint sync #5252

Merged
merged 8 commits into from
Feb 19, 2024
Merged

Conversation

michaelsproul
Copy link
Member

@michaelsproul michaelsproul commented Feb 17, 2024

Issue Addressed

Closes:

Proposed Changes

  • Download the blobs from the checkpoint sync URL.
  • Add a new flag --checkpoint-blobs which is not required at the CLI level but will trigger an error if not supplied when necessary.
  • Update tests (I think we can extend the weak subjectivity test in the beacon_chain crate)
  • Test manually against a Lighthouse BN.
  • Test manually against a checkpointz server (will log a warning until Serve blobs route ethpandaops/checkpointz#153 is implemented).

@michaelsproul michaelsproul added the work-in-progress PR is a work-in-progress label Feb 17, 2024
@michaelsproul michaelsproul changed the title MVP implementation (untested) Download checkpoint blobs during checkpoint sync Feb 17, 2024
@realbigsean realbigsean added the v5.0.0 Q1 2024 label Feb 18, 2024
@realbigsean
Copy link
Member

Here's some output from a manual test on prater:

Feb 18 09:05:40.409 INFO Starting checkpoint sync                remote_url: http://XXXXXXXXX:5052/, service: beacon
Feb 18 09:05:42.655 INFO Loaded checkpoint block and state       block_root: 0x1d153a6e2e327fefd0885d8b097e864f312d39feba58439e43f85c7e418cb0da, state_slot: 7644864, block_slot: 7644864, service: beacon

This block has a single commitment

curl localhost:5052/eth/v1/beacon/blocks/0x1d153a6e2e327fefd0885d8b097e864f312d39feba58439e43f85c7e418cb0da | jq | rg -A 2 blob_kzg
        "blob_kzg_commitments": [
          "0x887b07bd6427a95364900d2f4fa7b46da4d99338e8467dd904d915a0744c37d971b210b789c924a0edd43132de5123da"
        ]

A blob exists in the store for this block with a matching commitment

curl localhost:5052/eth/v1/beacon/blob_sidecars/0x1d153a6e2e327fefd0885d8b097e864f312d39feba58439e43f85c7e418cb0da | jq
{
  "data": [
    {
      "index": "0",
      "blob": "0x001611a..00000000"
      "kzg_commitment": "0x887b07bd6427a95364900d2f4fa7b46da4d99338e8467dd904d915a0744c37d971b210b789c924a0edd43132de5123da",
      "kzg_proof": "0xb01f7f61fbe0f1a9e95f155ddc116628f78f65851374f2fb0c6e4bb4e93b0297cc16ffff46919b20b6f66f2c4f8ba265",
      "signed_block_header": {
        "message": {
          "slot": "7644864",
          "proposer_index": "633346",
          "parent_root": "0x5985e81658a68a6f2de7c2cf20380a4a1679bc68ac019159dc9b24b8f73c385c",
          "state_root": "0x289327609039a0aca61b66bd055ef6fe1758b0f4f86e518790b7e31d4874c293",
          "body_root": "0xf7a5fffabb716ad0cb7e48eb08b6829e0a6d715c777775fea6d4a4109e66c0d1"
        },
        "signature": "0x96fc7c4bf681b69f8bd482404347b81b6eb07cdff0a13ac78887d885b599e860f97794018e86501d8d5a6fb3a32264c00e752198ca416303c2389cecff4220002f2657acef0da31317090585548ee601cee3ef75eed8ed8b2e123dc8e7e26edf"
      },
      "kzg_commitment_inclusion_proof": [
        "0x0000000000000000000000000000000000000000000000000000000000000000",
        "0xf5a5fd42d16a20302798ef6ed309979b43003d2320d9f0e8ea9831a92759fb4b",
        "0xdb56114e00fdd4c1f85c892bf35ac9a89289aaecb1ebd0a96cde606a748b5d71",
        "0xc78009fdf07fc56a11f122370658a353aaa542ed63e44c4bc15ff4cd105ab33c",
        "0x536d98837f2dd165a55d5eeae91485954472d56f246df256bf3cae19352a123c",
        "0x9efde052aa15429fae05bad4d0b1d7c64da64d03d7a1854a588c2cb8430c0d30",
        "0xd88ddfeed400a8755596b21942c1497e114c302e6118290f91e6772976041fa1",
        "0x87eb0ddba57e35f6d286673802a4af5975e22506c7cf4c64bb6be5ee11527f2c",
        "0x26846476fd5fc54a5d43385167c95144f2643f533cc85bb9d16b782f8d7db193",
        "0x506d86582d252405b840018792cad2bf1259f1ef5aa5f887e13cb2f0094f51e1",
        "0xffff0ad7e659772f9534c195c815efc4014ef1e1daed4404c06385d11192e92b",
        "0x6cf04127db05441cd833107a52be852868890e4317e6a02ab47683aa75964220",
        "0x0100000000000000000000000000000000000000000000000000000000000000",
        "0x792930bbd5baac43bcc798ee49aa8185ef76bb3b44ba62b91d86ae569e4bb535",
        "0x638b7115b2cdefa466348de967c308de1799c1e3d9ed2733422277c6dd59cdee",
        "0xdb56114e00fdd4c1f85c892bf35ac9a89289aaecb1ebd0a96cde606a748b5d71",
        "0x150d40bcc81f5bcce88cacd8add6e24ab4746a2822c27939beed92aaa268ae64"
      ]
    }
  ]
}

@realbigsean
Copy link
Member

I ran the same test using unstable to sanity check:

Feb 18 09:28:52.367 INFO Starting checkpoint sync                remote_url: http://XXXXXXXXX:5052/, service: beacon
Feb 18 09:28:53.916 INFO Loaded checkpoint block and state       block_root: 0x44b6dbc8daf269e08b4bbd667820a28c37535d143b7417d2bb7e06c5db2f0981, state_slot: 7644960, block_slot: 7644960, service: beacon

The checkpoint block contained 5 blobs

curl localhost:5052/eth/v1/beacon/blocks/0x44b6dbc8daf269e08b4bbd667820a28c37535d143b7417d2bb7e06c5db2f0981 | jq | rg -A 6 blob_kzg
        "blob_kzg_commitments": [
          "0xb4352e54d1609f92091766d852fdc92d9f95e0295f2ec5616227c1ebf52bee62f1b2a741f202963090697cd3380f76e8",
          "0xa7bf97e83e42fcfac89a010940d11c5e9e5fe33a53699aae53baef1b01711fa56e453f753fe77277f1f87eddefc5c794",
          "0xb17e7431edc890d22b1bcccd85a8f8d060d2143e4fe804ec8824f49b0edf3b7430ede15b78efa9a3d8440db1c0561518",
          "0xc00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
          "0xaa6d0b717eec8074cd84fed8ac17512ff7d7e62c8a08df637de9d5a1420a98b3292bc61594682e2022edf7168b590b92"
        ]

No blobs were present in the db for this block root

curl localhost:5052/eth/v1/beacon/blob_sidecars/0x44b6dbc8daf269e08b4bbd667820a28c37535d143b7417d2bb7e06c5db2f0981 | jq
{
  "data": []
}

@realbigsean
Copy link
Member

Update tests (I think we can extend the weak subjectivity test in the beacon_chain crate)

I made a very simple update here: 47902fa

just checking the checkpoint blobs we are initializing the chain with exist in the db

@realbigsean realbigsean added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Feb 18, 2024
paulhauner added a commit that referenced this pull request Feb 18, 2024
Squashed commit of the following:

commit a8a8121
Merge: a41d957 61aadeb
Author: realbigsean <sean@sigmaprime.io>
Date:   Sun Feb 18 03:56:55 2024 -0500

    Merge pull request #5253 from realbigsean/checkpoint-blobs-sean

    Checkpoint blobs sean

commit 61aadeb
Author: realbigsean <seananderson33@GMAIL.com>
Date:   Sun Feb 18 03:54:33 2024 -0500

    update cli help

commit 47902fa
Author: realbigsean <seananderson33@GMAIL.com>
Date:   Sun Feb 18 03:42:16 2024 -0500

    update store checkpoint sync test

commit a41d957
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Sat Feb 17 16:13:34 2024 +1100

    MVP implementation (untested)
@paulhauner paulhauner mentioned this pull request Feb 18, 2024
realbigsean
realbigsean previously approved these changes Feb 18, 2024
Copy link
Member

@realbigsean realbigsean left a comment

Choose a reason for hiding this comment

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

this looks good to me

@realbigsean
Copy link
Member

Test manually against a checkpointz server (will fail until ethpandaops/checkpointz#153 is implemented).

I'm realizing this means checkpointz doesn't work on any testnets where deneb already happened (so all of them).. I'm not sure we can merge this as is if that's the case. Maybe this PR should be updated to store blobs in the db if possible, and log a WARN otherwise? breaking checkpoint sync broadly on testnets might be pretty chaotic

@realbigsean realbigsean added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Feb 18, 2024
@realbigsean realbigsean dismissed their stale review February 18, 2024 12:24

rethinking it

@michaelsproul
Copy link
Member Author

breaking checkpoint sync broadly on testnets might be pretty chaotic

Yeah good point. I like your suggestion of just logging a warn if the endpoint returns 404. I can make this change in the morning (in 9h) if you like?

paulhauner added a commit that referenced this pull request Feb 18, 2024
Squashed commit of the following:

commit a8a8121
Merge: a41d957 61aadeb
Author: realbigsean <sean@sigmaprime.io>
Date:   Sun Feb 18 03:56:55 2024 -0500

    Merge pull request #5253 from realbigsean/checkpoint-blobs-sean

    Checkpoint blobs sean

commit 61aadeb
Author: realbigsean <seananderson33@GMAIL.com>
Date:   Sun Feb 18 03:54:33 2024 -0500

    update cli help

commit 47902fa
Author: realbigsean <seananderson33@GMAIL.com>
Date:   Sun Feb 18 03:42:16 2024 -0500

    update store checkpoint sync test

commit a41d957
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Sat Feb 17 16:13:34 2024 +1100

    MVP implementation (untested)
paulhauner added a commit that referenced this pull request Feb 18, 2024
Squashed commit of the following:

commit a8a8121
Merge: a41d957 61aadeb
Author: realbigsean <sean@sigmaprime.io>
Date:   Sun Feb 18 03:56:55 2024 -0500

    Merge pull request #5253 from realbigsean/checkpoint-blobs-sean

    Checkpoint blobs sean

commit 61aadeb
Author: realbigsean <seananderson33@GMAIL.com>
Date:   Sun Feb 18 03:54:33 2024 -0500

    update cli help

commit 47902fa
Author: realbigsean <seananderson33@GMAIL.com>
Date:   Sun Feb 18 03:42:16 2024 -0500

    update store checkpoint sync test

commit a41d957
Author: Michael Sproul <michael@sigmaprime.io>
Date:   Sat Feb 17 16:13:34 2024 +1100

    MVP implementation (untested)
@dapplion
Copy link
Collaborator

Yeah good point. I like your suggestion of just logging a warn if the endpoint returns 404. I can make this change in the morning (in 9h) if you like?

Strong preference to not crash on blob fetch 404 errors. Assuming checkpointz is not updated before mainnet fork, violating the db invariant should only cause peers to penalize the serving node. Since users checkpointz at scatered epochs it should be a big network wide issue. Not ideal but better than crashing.

@michaelsproul
Copy link
Member Author

I've pushed 2 commits to:

  • Just log a warning when the checkpoint server doesn't support the blobs endpoint. I've verified that this allows the node to start on Goerli when using a public checkpointz URL.
  • Verify that the block's commitments match the blobs. I've tested that this works with a local Lighthouse node as the checkpoint server. I've also tested manually using --checkpoint-blobs and SSZ blobs downloaded manually.

I've kept --checkpoint-blobs as mandatory when checkpoint syncing manually. Users syncing manually are probably sophisticated enough to fetch the blobs, and are (more) likely to value database integrity.

@michaelsproul michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Feb 19, 2024
Copy link
Member

@jimmygchen jimmygchen left a comment

Choose a reason for hiding this comment

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

Looks great!

@jimmygchen jimmygchen added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Feb 19, 2024
@paulhauner
Copy link
Member

@Mergifyio queue

Copy link

mergify bot commented Feb 19, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at c9702cb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-incompat Backwards-incompatible API change ready-for-merge This PR is ready to merge. v5.0.0 Q1 2024
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants