Cache hash for receipt block #700
Conversation
| // sufficient to uniquely identify the | ||
| // receipt block because of the uniqueness | ||
| // of nonce in receipts | ||
| pub hash: CryptoHash, |
There was a problem hiding this comment.
This has some issues with serialization:
hashshould be excluded from serialization, see: https://serde.rs/attr-skip-serializing.html- If
ReceiptBlockwas constructed through deserializationhashis not going to be computed and will have an incorrect default value. Instead we should compute hash lazily. Meaninghashshould beRefCell<Option<CryptoHash>>, thenHash::hashshould be checking if it isNone, and if it is then it computes it. It could have been easier if serde was supporting running custom code upon deserialization, but it currently does not: Add finalizer attribute hook to validate a deserialized structure serde-rs/serde#642
There was a problem hiding this comment.
Not sure if we want to do this right now. If so, we need to fix SignedTransaction, SignedBeaconBlock, SignedBeaconBlockHeader, SignedShardBlock, and SignedShardBlockHeader as well.
There was a problem hiding this comment.
In general, we need to move to computing hashes once, so at some point, we will have to do this modification to the structs that you have listed. However, since we have decided to do it to one struct at the moment, ReceiptBlock, then why not do it correctly now. Especially since, the modification that I proposed does not add much code. Had it involved adding a lot of code, I would have agreed with your argument.
There was a problem hiding this comment.
Since the SignedBlock trait requires Sync, we cannot use RefCell in ReceiptBlock. I tried to use Mutex but since Mutex does not implement Clone, it also cannot satisfy the requirement of SignedBlock.
There was a problem hiding this comment.
SignedBlock does not need neither Send nor Sync. I removed both of them and the project still compiles.
There was a problem hiding this comment.
It would fail here https://github.com/nearprotocol/nearcore/blob/3af2a57ba1029896013b4e7f523a530fb4cb1430/node/coroutines/src/client_task.rs#L412 with RefCell.
There was a problem hiding this comment.
I see, it would have to be Arc<RefCell<Option<CryptoHash>>. I think it is complex enough to be postponed to a different PR.
| // sufficient to uniquely identify the | ||
| // receipt block because of the uniqueness | ||
| // of nonce in receipts | ||
| pub hash: CryptoHash, |
There was a problem hiding this comment.
I see, it would have to be Arc<RefCell<Option<CryptoHash>>. I think it is complex enough to be postponed to a different PR.
No description provided.