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

Bootstrap #4430

Open
7 of 14 tasks
Tracked by #4415
Leo-Besancon opened this issue Sep 27, 2023 · 10 comments
Open
7 of 14 tasks
Tracked by #4415

Bootstrap #4430

Leo-Besancon opened this issue Sep 27, 2023 · 10 comments
Assignees
Labels
bootstrap Issues related to the bootstrap

Comments

@Leo-Besancon
Copy link
Contributor

Leo-Besancon commented Sep 27, 2023

Potential Problems

Improvements

  • P2 - Improve bootstrap universe #4540
  • P3 - add option on config.toml that sets the max_open_files configuration of rocksdb (default config < 1024)
  • P2 - Ensure receiving a non-valid db after bootstraping does not lead to a panic. Instead, we should restart bootstrap from scratch.
    In massa-node > src > main.rs:
     	if !final_state.read().is_db_valid() {
     		// TODO: Bootstrap again instead of panicking
     		panic!("critical: db is not valid after bootstrap");
     	}

Validation

  • P1 - ensure that a bootstrapping client can not make a bootstrap server panic by sending invalid data
  • P2 - make sure a client cannot deny service by staying connected too long or flooding
  • P2 - implement unit tests for those aspects
  • P2 - validate bootstrap server/client messages serialization / deserialization
  • P3 - calibrate bandwidth limits and timeout values (after ensuring BootstrapMessage serialization limits are working well)
  • P2 - calibrate U64VarIntDeserializer max bound for state_new_elements_length_deserializer (same)
  • P2 - Limit the number of updates we send during Bootstrap. We keep sending all the changes, but if they are too big, we should send an error to the client so that they can bootstrap again from scratch. (same)
  • P1 - test that messages never exceeds limits in size
@litchipi
Copy link
Contributor

If we start a labnet without keeping the ledger, just a normal one without any operations on the branch test_bootstrap_big, the bootstrap process will work but the hashes of final_state will be different.

It seams that some changes I added in the branch test_bootstrap_big is responsible for the hash difference

@litchipi
Copy link
Contributor

I figured out why the final state hash are different when the node generates a brand new ledger, it works now.
However, it's still different when a ledger is generated using massa-ledger-editor

@litchipi
Copy link
Contributor

litchipi commented Sep 28, 2023

The hash mismatch occurs when launching a labnet on main with crafted ledger.
After I tried with smaller keys / values in datastore (length 9000), it still does it, so I don't think it's a matter of serialization.
@Leo-Besancon Could it be because of this piece of code we use when crafting the ledger ?
How could I modify the massa-ledger-editor in order to craft a ledger that doesn't require this check bypass ?

EDIT: Disregard this comment, I made a mistake while writing it, see below

@Leo-Besancon
Copy link
Contributor Author

The hash mismatch occurs when launching a labnet on main with crafted ledger. After I tried with smaller keys / values in datastore (length 9000), it still does it, so I don't think it's a matter of serialization. @Leo-Besancon Could it be because of this piece of code we use when crafting the ledger ? How could I modify the massa-ledger-editor in order to craft a ledger that doesn't require this check bypass ?

Nice research! I think the easiest to ensure that all necessary keys are found would be to start from a working ledger (built normally by the node), and then only add your crafted keys.
This way, the trail_hash, cycle_history and so one would be in your crafted ledger, and it's less likely to cause a problem.

@litchipi
Copy link
Contributor

That's actually what I already do

  • A while ago I started a node normally, and copied the storage into a local folder A.
  • To craft a new ledger, I copy the content of the folder A to a new folder B
  • Using massa-ledger-editor, I add data to the ledger stored in ledger B
  • Before I launch the labnet, I copy the folder B to the storage of the massa-node.

(Note that most of what I listed above is automated so I don't forget a step while doing it)

So the whole basis of the ledger comes from an (almost) empty ledger generated by the massa-node.
I only add new entries (new addresses with datastore and bytecode associated), using the massa libraries.
Normally, the only invalid data is the random generated data (datastore keys + value, and the bytecode), the rest is "standard"

@litchipi
Copy link
Contributor

@Leo-Besancon My bad, I made a mistake while writing the message, when using smaller datastore key + value, the hashes DOES MATCH, so it seams that's it's linked to the size of the data inside the ledger.
Either the serialization, transmission, reception or deserialization of the data isn't calibrated for such a size

@Leo-Besancon
Copy link
Contributor Author

Ok thanks for the clarification! In that case testing the message serialization should be the next step? :)

@litchipi
Copy link
Contributor

litchipi commented Oct 4, 2023

Update:

  • The Serialization / Deserialization of the BootstrapServerMessage has been tested with limit cases, and it's wokring OK
  • When adding to the ledger a specially crafted key in the datastore, and displaying its value after bootstrap, the value is the one expected, but the final_state hash is different after bootstrap (expected). So this test is OK also
  • When adding to the ledger a specially crafted bytecode, and displaying it after bootstrap, the value is the one expected, but the final_state hash is different after bootstrap (expected). So this test is OK also

@litchipi
Copy link
Contributor

litchipi commented Oct 4, 2023

I'm confused, I just tested again inserting a single data with length 500 and it DOESN'T work anymore...
It also don't work anymore when simply passing an empty ledger, with the keep-ledger option...

From now on, I think I'll have to consider that any ledger crafted using massa-ledger-editor is potentially bad.

bors bot added a commit that referenced this issue Oct 24, 2023
4453: Bootstrap improvements r=litchipi a=litchipi

Take most of the work done in #4420 to merge it in `main`

Replace the `MAX_BOOTSTRAP_NEW_ELEMENTS` with a size limit `MAX_BOOTSTRAP_NEW_ELEMENTS_SIZE` so that voluminous messages (because of big datastore values or bytecodes) can be transferred without going through the limit.

Limits on the timeouts, and message size had to be raised in order to allow messages crafted to be very voluminous to be transferred without any problem.
They may not have to be reduced down, but high enough to allow a big ledger to bootstrap successfully.
This will have to be done after all the issues documented in #4430 and #4406 

Co-authored-by: Litchi Pi <litchi.pi@proton.me>
Co-authored-by: Litchi Pi (Tim) <litchi.pi@proton.me>
bors bot added a commit that referenced this issue Oct 24, 2023
4453: Bootstrap improvements r=litchipi a=litchipi

Take most of the work done in #4420 to merge it in `main`

Replace the `MAX_BOOTSTRAP_NEW_ELEMENTS` with a size limit `MAX_BOOTSTRAP_NEW_ELEMENTS_SIZE` so that voluminous messages (because of big datastore values or bytecodes) can be transferred without going through the limit.

Limits on the timeouts, and message size had to be raised in order to allow messages crafted to be very voluminous to be transferred without any problem.
They may not have to be reduced down, but high enough to allow a big ledger to bootstrap successfully.
This will have to be done after all the issues documented in #4430 and #4406 

Co-authored-by: Litchi Pi <litchi.pi@proton.me>
Co-authored-by: Litchi Pi (Tim) <litchi.pi@proton.me>
@litchipi litchipi removed their assignment Nov 6, 2023
@litchipi
Copy link
Contributor

litchipi commented Nov 6, 2023

@sydhds Some of the tests you are doing are ticking the boxes on this issue I think

@AurelienFT AurelienFT added the bootstrap Issues related to the bootstrap label Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bootstrap Issues related to the bootstrap
Projects
None yet
Development

No branches or pull requests

3 participants