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.

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

Private state update metadata and migration #354

Closed
wants to merge 24 commits into from
Closed

Private state update metadata and migration #354

wants to merge 24 commits into from

Conversation

lucassaldanha
Copy link
Member

@lucassaldanha lucassaldanha commented Feb 2, 2020 •
edited
Loading

PR description

• Updated the private state storage with new data structures that aren't backwards compatible.
• Added PrivateStateRootResolver that handles public chain forks and their impact on the private state
• Created migration logic that will process previous private txs and re-create the data in the update 1.4.x format.
• Migration is based on the schema version, defined in the PrivateStateStorage
• Added flag that user needs to "opt-in" to migrate the data
• Remove restriction for "latest" block from priv_call

@lucassaldanha lucassaldanha self-assigned this Feb 2, 2020
@lucassaldanha lucassaldanha added the privacy private transactions label Feb 2, 2020
@lucassaldanha
Fix migration of failed txs
c28fb51 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Remove restriction for priv_call
2d7ea37 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Merge branch 'master' into updated-migration
b6d2855 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Fix import
73adcf1 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Merge branch 'master' into updated-migration
1740211 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Fix unit test
5111882 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Changed migration msg
aa0f1a6 
Signed-off-by: Lucas Saldanha <[email protected]>
pinges
pinges reviewed Feb 6, 2020
View reviewed changes
besu/src/main/java/org/hyperledger/besu/cli/BesuCommand.java
final PrivacyParameters privacyParameters = privacyParametersBuilder.build();

if (isPrivacyEnabled) {
preSynchronizationTaskRunner.addTask(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that how it is done in besu? Why not check whether migratePrivateDatabase is true and then add the task?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because part of the task is to evaluate the different triggering conditions. And one of the conditions is: you have a private db that needs migration but you don't have the migratePrivateDatabase flag enabled.

besu/src/main/java/org/hyperledger/besu/cli/presynctasks/PreSynchronizationTask.java Outdated
import org.hyperledger.besu.controller.BesuController;

/**
* All PreSynchronizationTask insatnces execute after the {@link BesuController} instance in {@link
Copy link
Contributor

Choose a reason for hiding this comment

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

s/insatnces/instances/

...in/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/PrivacyApiGroupJsonRpcMethods.java
@@ -55,6 +59,14 @@ public PrivacyApiGroupJsonRpcMethods(
this.protocolSchedule = protocolSchedule;
this.transactionPool = transactionPool;
this.privacyParameters = privacyParameters;

final PrivateStateRootResolver privateStateRootResolver =
new PrivateStateRootResolver(privacyParameters.getPrivateStateStorage());
Copy link
Contributor

Choose a reason for hiding this comment

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

inline?

...in/java/org/hyperledger/besu/ethereum/api/jsonrpc/methods/PrivacyApiGroupJsonRpcMethods.java Outdated
@@ -73,6 +85,10 @@ public PrivacyParameters getPrivacyParameters() {
return privacyParameters;
}

public PrivateNonceProvider getPrivateNonceProvider() {
Copy link
Contributor

Choose a reason for hiding this comment

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

IntelliJ says this method is never called. Do we need it? And the privateNonceProvider?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe we don't need getPrivateNonceProvider() anymore. We definitely need PrivateNonceProvider though. :)
I have removed the method.

ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/PrivacyBlockProcessor.java Outdated
final BlockHeader blockHeader,
final List<Transaction> transactions,
final List<BlockHeader> ommers) {
final PrivacyGroupHeadBlockMap privacyGroupHeadBlockHash =
Copy link
Contributor

Choose a reason for hiding this comment

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

s/privacyGroupHeadBlockHash/privacyGroupHeadBlockMap/

besu/src/test/java/org/hyperledger/besu/PrivacyReorgTest.java
privateStateRootResolver, blockchain, STATE_ROOT_AFTER_TRANSACTION_APPENDED_TO_EMTPY_STATE);

final String secondForkBlockStateRoot =
"0xd35eea814b8b5a0b12e690ab320785f3a33d9685bbf6875637c40a64203915da";
Copy link
Contributor

Choose a reason for hiding this comment

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

constant

besu/src/test/java/org/hyperledger/besu/PrivacyReorgTest.java

// Add another private transaction
final String thirdForkBlockStateRoot =
"0xe22344ade05260177b79dcc6c4fed8f87ab95a506c2a6147631ac6547cf44846";
Copy link
Contributor

Choose a reason for hiding this comment

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

constant

besu/src/test/java/org/hyperledger/besu/PrivacyReorgTest.java Outdated
parentBlock);
}

private BlockDataGenerator.BlockOptions getBlockOptionsNoTransaction(
Copy link
Contributor

Choose a reason for hiding this comment

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

this block will have a transaction!!! Fix the name!

besu/src/test/java/org/hyperledger/besu/PrivacyReorgTest.java
.setEnclaveUrl(enclave.clientUrl())
.setEnclaveFactory(new EnclaveFactory(Vertx.vertx()))
.build();
privacyParameters.setEnclavePublicKey("A1aVtMxLCUHmBVHXoZzzBgPbW/wj5axDpW9X8l91SGo=");
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the constant ENCLAVE_PUBLIC_KEY

Copy link
Contributor

Choose a reason for hiding this comment

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

In lines 154 and 410 we are using the String for that key as a parameter. Can we have a constant for the String as well (ENCLAVE_PUBLIC_KEY_STRING) to make clear what we are doing?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can use ENCLAVE_PUBLIC_KEY.toBase64String()

besu/src/test/java/org/hyperledger/besu/PrivacyReorgTest.java
@@ -0,0 +1,540 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

would be nice if we could create constants for the "magic" numbers, but if we cannot do that we should provide some kind of clue where these numbers come from ...

pinges
pinges reviewed Feb 10, 2020
View reviewed changes
...ger/besu/ethereum/privacy/storage/migration/PrivateStorageMigrationTransactionProcessor.java Outdated
return Optional.empty();
}

final MutableWorldState publicWorldState =
Copy link
Contributor

Choose a reason for hiding this comment

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

When we are simulating this private transaction we need to use the public state at the end of the public Tx just before the PMT.

@lucassaldanha
Minor PR comments
ea2af20 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Reorg test
6fce1b2 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Spotless
65704c2 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Fix unused warnings
118c7e5 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Revert "Fix unused warnings"
9652a9d 
This reverts commit 118c7e5

Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Fix test
16d938a 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Handling enclave error
57194da 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Merge branch 'master' into updated-migration
2653938 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Merge
e095b5b 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Fix AT
ca11476 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
[draft] private metadata migration v2
177982f 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Updated migration logic
5c5b83c 
Signed-off-by: Lucas Saldanha <[email protected]>
@lucassaldanha
Copy link
Member Author

Superseeded by #404

@lucassaldanha lucassaldanha deleted the updated-migration branch February 17, 2020 02:18
siladu pushed a commit to siladu/besu that referenced this pull request Oct 28, 2024
@ralexstokes
CL-EL withdrawals harmonization: using units of Gwei (hyperledger#354)
5561353
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinges pinges pinges left review comments

@iikirilov iikirilov Awaiting requested review from iikirilov

Assignees

@lucassaldanha lucassaldanha

Labels
privacy private transactions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lucassaldanha @pinges