-
Notifications
You must be signed in to change notification settings - Fork 280
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
refactor!: Use hash to validate genesis block #4793
Conversation
So if we still have to use genesis account afaik what benefits do we gain by now also checking hash of genesis block? |
As I see |
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
Signed-off-by: Dmitry Murzin <diralik@yandex.ru>
cf0c402
to
1a748d2
Compare
Prepare(prepare::Args), | ||
Generate(generate::Args), |
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.
Prepare(prepare::Args), | |
Generate(generate::Args), | |
Prepare(prepare::Args), | |
Generate(generate::Args), |
do we need both?
@@ -329,7 +339,6 @@ mod candidate { | |||
impl SignedTransactionCandidate { | |||
fn validate(self) -> Result<SignedTransactionV1, &'static str> { | |||
self.validate_instructions()?; | |||
self.validate_signature()?; |
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.
I don't like this. Transactions with incorrect signatures might creep into Iroha. I'd rather that you make another SignedTransactionCandidate
in data_model::block
which will have different implementation, i.e. it will not verify genesis signatures
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.
I agree, but what to do with validate_transaction
in executor? It will deserialize SignedTransaction
at WASM side, so it will fail for genesis transaction if we keep validate signature during deserialization
One option I can think of is to make SignedTransaction
validation #[cfg(not(target_family = "wasm"))]
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.
One option I can think of is to make
SignedTransaction
validation#[cfg(not(target_family = "wasm"))]
I like this suggestion. Can we say that since data model entities are always provided by the host there is no need to validate them on the wasm side? If so, we can can open a ticket to generalize this approach to all entities, although it seems kinda hacky and I'm not sure if this won't bite us in the ass
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.
Another possibility is to hardcode some public key as genesis and say that every Iroha deployment will have this as genesis account_id. In that case we can remove account
from genesis.json
. This is more or less how we had it except that kagami
wouldn't have to receive private key but it already knows it's value. @Erigara @s8sato ?
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.
let's discuss tomorrow on standup
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 can use the same multihash format but with identity
code
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.
After discussion it was noted that we still want genesis to sign transaction/block in which case my proposal is not relevant.
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.
The complexity seems to come from trying to strip away the functionality of digital signatures while taking advantage of the transaction processing flow.
There might not have been any problem in the first place: if the privilege comes from block height 0, what is the actual harm even if the genesis account with the correct private key appears after genesis?
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.
what is the actual harm even if the genesis account with the correct private key appears after genesis?
I like this point. Especially if someone has misconfigured the blockchain in genesis.json, for example they haven't transfered ownership
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.
Yeah, currently custom genesis can grant any privileges to any account. But they agreed anyway. They joined the network while being able to see that genesis block
if transaction.value.authority().domain.name.as_ref() != "genesis" { | ||
return Err("Genesis transaction authority must be from `genesis` domain"); | ||
} |
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.
I don't think we should care about this?
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.
I think we can remove GENESIS_DOMAIN_ID
check from this module. Also I think that we can remove this constant alltogether and with it the notion of "genesis" domain
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.
There are checks like "not allowed to register account in genesis domain" in impl Execute for Register<Account>
. Should we keep those checks?
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.
I don't think we need to keep those checks. Because if we do, we have to track it in runtime which is complicated. Remove checks and open an issue to discuss
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.
I agree, after genesis there doesn't seem to be any particular harm in referring a domain named "genesis"
@@ -139,5 +139,6 @@ | |||
} | |||
} | |||
], | |||
"topology": [] | |||
"topology": [], | |||
"genesis": "ed01204164BF554923ECE1FD412D241036D863A6AE430476C898248B8237D77534CFC4@genesis" |
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.
"genesis": "ed01204164BF554923ECE1FD412D241036D863A6AE430476C898248B8237D77534CFC4@genesis" | |
"account": "ed01204164BF554923ECE1FD412D241036D863A6AE430476C898248B8237D77534CFC4@genesis" |
let's rename, it looks silly. Sorry, it was my initial suggestion
TRUSTED_PEERS: '[{"address":"irohad2:1339","public_key":"ed01204EE2FCD53E1730AF142D1E23951198678295047F9314B4006B0CB61850B1DB10"},{"address":"irohad0:1337","public_key":"ed0120A98BAFB0663CE08D75EBD506FEC38A84E576A7C9B0897693ED4B04FD9EF2D18D"},{"address":"irohad3:1340","public_key":"ed0120CACF3A84B8DC8710CE9D6B968EE95EC7EE4C93C85858F026F3B4417F569592CE"}]' | ||
TOPOLOGY: '[{"address":"irohad2:1339","public_key":"ed01204EE2FCD53E1730AF142D1E23951198678295047F9314B4006B0CB61850B1DB10"},{"address":"irohad1:1338","public_key":"ed01209897952D14BDFAEA780087C38FF3EB800CB20B882748FC95A575ADB9CD2CB21D"},{"address":"irohad0:1337","public_key":"ed0120A98BAFB0663CE08D75EBD506FEC38A84E576A7C9B0897693ED4B04FD9EF2D18D"},{"address":"irohad3:1340","public_key":"ed0120CACF3A84B8DC8710CE9D6B968EE95EC7EE4C93C85858F026F3B4417F569592CE"}]' |
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.
what was the reason you introduced genesis to all peers? is it because of the integration tests?
On the other hand, I would prefer keeping it as simple as possible because of external users
@@ -417,7 +412,7 @@ mod valid { | |||
block: SignedBlock, | |||
topology: &Topology, | |||
expected_chain_id: &ChainId, | |||
genesis_account: &AccountId, | |||
genesis_hash: &HashOf<SignedBlock>, |
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.
genesis_hash: &HashOf<SignedBlock>, | |
genesis_hash: HashOf<SignedBlock>, |
it's a copy type
fn genesis_account_and_domain(public_key: PublicKey) -> (Account, Domain) { | ||
let genesis_account_id = | ||
AccountId::new(iroha_genesis::GENESIS_DOMAIN_ID.clone(), public_key); | ||
let genesis_account = Account::new(genesis_account_id).into_account(); | ||
let genesis_domain = | ||
Domain::new(iroha_genesis::GENESIS_DOMAIN_ID.clone()).build(&genesis_account.id); | ||
(genesis_account, genesis_domain) |
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.
fn genesis_account_and_domain(public_key: PublicKey) -> (Account, Domain) { | |
let genesis_account_id = | |
AccountId::new(iroha_genesis::GENESIS_DOMAIN_ID.clone(), public_key); | |
let genesis_account = Account::new(genesis_account_id).into_account(); | |
let genesis_domain = | |
Domain::new(iroha_genesis::GENESIS_DOMAIN_ID.clone()).build(&genesis_account.id); | |
(genesis_account, genesis_domain) | |
fn genesis_account_and_domain(public_key: PublicKey) -> (Account, Domain) { | |
let genesis_account_id = | |
AccountId::new(iroha_genesis::GENESIS_DOMAIN_ID.clone(), public_key); | |
let genesis_account = Account::new(genesis_account_id).into_account(); | |
let genesis_domain = | |
Domain::new(iroha_genesis::GENESIS_DOMAIN_ID.clone()).build(&genesis_account.id); | |
(genesis_account, genesis_domain) |
IMO we should take both from the genesis authority. And remove the GENESIS_DOMAIN_ID
} | ||
|
||
/// Extracts inner `SignedBlock` | ||
pub fn into_inner(self) -> SignedBlock { |
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.
why not just implement From<GenesisBlock> for SignedBlock
@@ -23,14 +23,31 @@ pub static GENESIS_DOMAIN_ID: Lazy<DomainId> = Lazy::new(|| "genesis".parse().un | |||
/// First transaction should contain single [`Upgrade`] instruction to set executor. | |||
/// Second transaction should contain all other instructions. | |||
/// If there are no other instructions, second transaction will be omitted. | |||
#[derive(Debug, Clone)] | |||
#[derive(Debug, Clone, Encode)] |
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.
why do we need Encode
now?
// There is no need to sign genesis transactions since we check it by hash, | ||
// but our data model requires SignedTransaction to be signed, | ||
// so we sign it with random key pair | ||
.sign(KeyPair::random().private_key()) |
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 to bring attention for other reviewers. This is a bit contentious, but I think it's a good approach.
/// Genesis account | ||
genesis: AccountId, |
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.
please put it after chain
. Generated json file will look nicer
Discussed with @mversic and decided that currently it is not worth to implement #4555. Originally it was expected that it will simplify things, but looks like there is no good and clean implementation, so will keep current approach with genesis public and private key (note that genesis private key is used only in kagami) |
Description
genesis.hash
config parameter and removedgenesis.public_key
genesis
field (account id) togenesis.json
. It will be authority of genesis transactionskagami genesis sign
withkagami genesis prepare
which convertsgenesis.json
togenesis.scale
(SignedBlock
) and outputs genesis hash to stdoutWorld
after receiving genesis block (since we now don't know genesis account id at startup but need to use authority of genesis block)Todo:
Deployment changes
genesis
field togenesis.json
(genesis account id)kagami genesis sign
invocationBefore:
After:
This will also output genesis hash
GENESIS_HASH
environment variable toirohad
Linked issue
Closes #4555
Benefits
Checklist
CONTRIBUTING.md