Authenticated RawDB calls for Accessors Chain File#2
Conversation
Sneh1999
changed the title
Sneh1999
force-pushed
the
authenticated-rawdb-calls
branch
from
a1edc4a to
7e6ff52
Compare
Sneh1999
force-pushed
the
authenticated-rawdb-calls
branch
from
7e6ff52 to
a1dc0a0
Compare
Sneh1999
changed the title
…into authenticated-rawdb-calls
alxiong
left a comment
alxiong
left a comment
There was a problem hiding this comment.
(take a grain of salt, I've never touch geth codebase, so my comments might be off due to lack of understanding)
Currently I see two big issues:
- ambiguous data types.
Hashtype erase what kind of hash we are expecting, body hash (viacommon.BytesToHash(bodybytes)or block hash (which is the hash of the body viabody.Header().Hash()orblock.Hash()). We should make it explicitly clear either in function name or in argument name. - there's lots of duplication in verifying the same block many times because we add
VerifyBlockSignature()in many rawdb functions which has inter-dependency. I think we should map out the call graph of all rawdb functions inaccessors_chain.goand only add verify at the lowest/root level in the call graph.
| err = StoreBlockSignatureForTests(db, h, signature) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to store signature: %v", err) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func StoreBlockSignatureForTests(db ethdb.KeyValueWriter, blockHash common.Hash, blockSignature []byte) error { | ||
| blockNumber := binary.BigEndian.Uint64(blockHash.Bytes()) | ||
| key := dbKey(BlockSignaturePrefix, (blockNumber)) | ||
| return db.Put(key, blockSignature) | ||
| } |
There was a problem hiding this comment.
StoreBlockSignatureForTestsseems only internal use, make it private?- can you explain the design rationale for the db key derivation? why do i have to convert the blockHash to a number first, what's wrong with using it directly as bytes?
func GetBlockSignature(db ethdb.KeyValueReader, blockHash common.Hash) ([]byte, error) {
key := append(BlockSignaturePrefix, blockHash.Bytes()...)
return db.Get(key)
}There was a problem hiding this comment.
I understand why now. it's because how we generate them (which imo should be revised):
https://github.com/EspressoSystems/nitro-espresso-integration/blob/a32258957edd831ecfd41da5b3078bcaf611f98e/arbnode/espresso_utils.go#L102-L105
|
|
||
| // VerifyBodyMatchesBlockHashProof verifies that the given body matches the block hash which | ||
| // the enclave has signed over. | ||
| func VerifyBodyMatchesBlockHashProof(db ethdb.Reader, number uint64, hash common.Hash, body *types.Body) error { |
There was a problem hiding this comment.
so, this function name and what it does doesn't match (? imo)
I thought there should be at least GetBlockSignature, and then verify the proof? but there's no proof involved at all in the body of this func.
a clear evidence is: why would you try to get SNAPSHOT_ADDRESS but never use it, but only testing if it's non-empty?
same thing for VerifyReceiptsInBlock() and VerifyLogsInBlock().
There was a problem hiding this comment.
I was just checking if SNAPSHOT_ADDRESS is set to see if its running in TEE or not because the same code needs to work outside the TEE as well.
The proof is actually happening here because we are verifying the body of the block is indeed part of the canonical header whose hash we have signed
| return nil | ||
| } | ||
|
|
||
| func VerifyBlockNumber(db ethdb.Reader, number uint64, hash common.Hash) error { |
There was a problem hiding this comment.
its used in HasBody
|
Ah! another thing I forgot to mention is: should we use MAC or signature? i'm sure KMS also support HMAC algorithm and managing MAC key. |
I dont know much about this so cant comment a lot all I can say is that because geth heavily uses ECDSA signatures so maybe we should also use that but I am no cryptography expert so @alxiong whatever you think is the best let me know! |
|
imo, we should definitely use HMAC over signature, this is the definition usage of MAC, and much more efficient than signature, HMAC is basically bunch of hashes which is super fast, no expensive curve operations. Also KMS's HMAC is RFC standard, so compatible with go/crypto's impl: https://docs.aws.amazon.com/kms/latest/developerguide/hmac.html let me demonstrate in my next PR. |
2 participants
Desciption
This is the first PR in geth to add authenticated storage. It updates the first file in rawdb to make some of the rawdb calls authenticated. It adds utility functions to verify merkle proofs over the transaction, receipts and bloom filters
Testing
Important places to review
core/rawdb/accessors_chain.gofile carefully and check I am verifying everything correctly 🙏