-
Notifications
You must be signed in to change notification settings - Fork 834
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
feat: Expose set finalized/safe block in plugin api BlockchainService #7382
feat: Expose set finalized/safe block in plugin api BlockchainService #7382
Conversation
…vice Signed-off-by: Usman Saleem <[email protected]>
…ainservice_finalized_block Signed-off-by: Usman Saleem <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty straightforward, just thinking if we need to support some kind of checks, and allow to set these values only if we are not running a PoS network?
…ainservice_finalized_block Signed-off-by: Usman Saleem <[email protected]>
…ainservice_finalized_block Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
* @param blockHash Hash of the finalized block | ||
* @throws UnsupportedOperationException if the network is not a POA network | ||
*/ | ||
void setFinalizedBlock(Hash blockHash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also need the setSafeBlock method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
@@ -135,6 +142,17 @@ public Optional<Hash> getFinalizedBlock() { | |||
return blockchain.getFinalized(); | |||
} | |||
|
|||
@Override | |||
public void setFinalizedBlock(final Hash blockHash) { | |||
if (genesisConfigOptionsSupplier != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking the genesis isn't going to work for chains that transition to POS.
You could use the protocolSchedule to check the chain is POS for given blockHash by doing something like
protocolSchedule.getByBlockHeader(blockchain.getBlockHeader(blockHash).get()).isPoS()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
Signed-off-by: Usman Saleem <[email protected]>
…ainservice_finalized_block Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM: consider adding few tests
…ainservice_finalized_block Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
Signed-off-by: Usman Saleem <[email protected]>
|
||
@Test | ||
@DisplayName("Calling updateFinalizedBlockV1 will set finalized block") | ||
public void canUpdateFinalizedBlock() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also test the updateSafeBlock method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
Signed-off-by: Usman Saleem <[email protected]>
…ainservice_finalized_block Signed-off-by: Usman Saleem <[email protected]>
…ainservice_finalized_block Signed-off-by: Usman Saleem <[email protected]>
…hyperledger#7382) * feat: Expose set finalized and safe block in plugin-api BlockchainService * check for poa network before setting finalized block * changelog * Add BlockchainService set finalized acceptance test --------- Signed-off-by: Usman Saleem <[email protected]> Signed-off-by: gconnect <[email protected]>
PR description
feat: Expose set finalized/safe block in plugin api BlockchainService. This method can be used by plugins to set finalized/safe block for a POA network (such as QBFT, IBFT and Clique).
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-required
label to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew build
./gradlew acceptanceTest
./gradlew integrationTest
./gradlew ethereum:referenceTests:referenceTests