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

PoA Block Signing #718

Merged
merged 13 commits into from
Oct 26, 2022
Merged

PoA Block Signing #718

merged 13 commits into from
Oct 26, 2022

Conversation

Voxelot
Copy link
Member

@Voxelot Voxelot commented Oct 25, 2022

Initial implementation of PoA block signing & persistence of fuel consensus data.

Todo:

  • update devops configs to accept a consensus secret (deferred to followup PR)
  • (deferrable) safe method to convert block id's into messages for signatures (may require a BlockId newtype)
  • (deferrable) update fuel-crypto to support zeroize on secret types

In the future, we should use the keystore traits provided by fuel-crypto to allow for a pluggable signing backend.

closes: #668
closes: #673

@Voxelot Voxelot requested a review from a team October 25, 2022 04:49
@Dentosal
Copy link
Member

Zeroize support in fuel-crypto has been merged. A non-breaking release of fuel-crypto should be doable, so it requires no changes here.

* Create a BlockId type for safe conversion to Message

* fmt

* Clippy

* Remove direct dep to fuel-crypto, as it's re-exported by fuel-vm

* Allow Into impl for clippy

* Fix lint name
@xgreenx
Copy link
Collaborator

xgreenx commented Oct 25, 2022

Also, maybe we can clean up "TODO: Do not use." and related code=)

@Voxelot Voxelot marked this pull request as ready for review October 26, 2022 01:17
@Voxelot
Copy link
Member Author

Voxelot commented Oct 26, 2022

Will defer devops configuration of the secret key to a follow-up PR pending some investigation.

@Voxelot Voxelot requested a review from a team October 26, 2022 01:19
Copy link
Contributor

@freesig freesig left a comment

Choose a reason for hiding this comment

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

Mostly looks good, just a few suggestions

fuel-core-interfaces/src/model/block.rs Outdated Show resolved Hide resolved
}

impl Database {
pub fn get_sealed_block(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check for mismatches here? Like the header having a different consensus type then the FuelBlockConsensus?

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 could, although there's not really much the application can do if it gets into that state 🤔

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 think we should defer this checking until we have more than one kind of consensus variant, as rust won't let me even write the error case right now.

fuel-core/src/service/config.rs Show resolved Hide resolved
fuel-core/src/database.rs Outdated Show resolved Hide resolved
fuel-core/src/database.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@freesig freesig left a comment

Choose a reason for hiding this comment

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

Looks good

@Voxelot Voxelot merged commit fa869ad into master Oct 26, 2022
@Voxelot Voxelot deleted the block-signing branch October 26, 2022 06:53
MujkicA pushed a commit that referenced this pull request Oct 26, 2022
* add block signing and db models for sealed blocks

* add integ tests for checking sealed blocks

* mock setting the block height after production

* Create a BlockId type for safe conversion to Message (#719)

* Create a BlockId type for safe conversion to Message

* fmt

* Clippy

* Remove direct dep to fuel-crypto, as it's re-exported by fuel-vm

* Allow Into impl for clippy

* Fix lint name

* replace [u8;32] with secret key & secret key wrapper

* clippy

* use derive_more for newtype traits

* remove unneeded duplicate insert check

* add as_message to block id & block_producer getter to consensus data

* use real consensus data in the Interpreter::coinbase method

* add error context

* wean off of deref()

Co-authored-by: Hannes Karppila <hannes.karppila@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SealedFuelBlock DB model PoA Block Signing
5 participants