Nuke cows in Receipt #12470
Nuke cows in Receipt #12470
Conversation
| @@ -491,10 +490,11 @@ impl DelayedReceiptQueueWrapper { | |||
| true => { | |||
| let metadata = | |||
| StateStoredReceiptMetadata { congestion_gas: gas, congestion_size: size }; | |||
| let receipt = StateStoredReceipt::new_borrowed(receipt, metadata); | |||
| // FIXME eagr | |||
| let receipt = StateStoredReceipt::new(receipt.clone(), metadata); | |||
There was a problem hiding this comment.
Are these clones gonna be deal breaker? @wacban
There was a problem hiding this comment.
It would be great if we could avoid cloning receipts. Cloning a receipt is a dangerous operation because it duplicates the operation that will be performed when the receipt is executed. For example duplicating a transfer receipt could potentially lead to double spending.
Ideally the receipts would never be cloned. Pushing a receipt onto a queue should consume the receipt. This way the compiler would ensure that a receipt is never cloned and receipts are never duplicated.
I'm not a fan of cloning as a solution for Cow, is it possible to refactor the code so that the receipts are consumed?
There was a problem hiding this comment.
I'm not a fan of cloning as a solution for Cow, is it possible to refactor the code so that the receipts are consumed?
I was entirely not sure those either, kinda wanna go the naive way first to see what it breaks to gain a better idea of why Cow was used in the first place. Yeah, I'll see what I could do
There was a problem hiding this comment.
If the receipts can't be consumed because information about them is needed later, maybe it would be possible to save the information and consume the receipt? Like having a queue which consumes the Receipt and returns ReceiptInfo that is used later. ReceiptInfo would contain the needed information, while not being a cloned receipt that can be executed.
I don't remember why exactly there was a problem with consuming them.
There was a problem hiding this comment.
Agree with jancio, the clones were introduced precisely in order to avoid clone on receipts.
Agree with jancio again, the receipt should be consumed, I think only some metadata about it is needed after it's been stored in the StateStoredReceipt. That would require some refactoring.
@eagr It's a great first step! I think now you can just make the receipt consumed by removing reference, see what breaks and refactor as needed. IIRC there was only one such place, where we need some simple stats about local receipts.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12470 +/- ##
==========================================
- Coverage 71.45% 71.45% -0.01%
==========================================
Files 838 838
Lines 169343 169332 -11
Branches 169343 169332 -11
==========================================
- Hits 121004 120996 -8
+ Misses 42993 42991 -2
+ Partials 5346 5345 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
There is gonna be a merge conflict with #12464 which adds |
| #[derive(PartialEq, Eq, Debug, ProtocolSchema)] | ||
| pub enum ReceiptOrStateStoredReceipt<'a> { | ||
| Receipt(Cow<'a, Receipt>), | ||
| StateStoredReceipt(StateStoredReceipt<'a>), | ||
| pub enum ReceiptOrStateStoredReceipt { | ||
| Receipt(Receipt), | ||
| StateStoredReceipt(StateStoredReceipt), | ||
| } |
There was a problem hiding this comment.
The protocol schema CI check is failing because structs with ProtocolSchema derive were updated without updating the checksum file. This is a sanity check to ensure that the serialization format of structs that are part of the protocol doesn't accidentally change. Please ensure that modifying the struct doesn't change the serialization format (you can borsh serialize it before and after the change and compare) and update the checksums.
See: https://github.com/near/nearcore/tree/master/tools/protocol-schema-check
There was a problem hiding this comment.
could confirm that removing the Cows doesn't change the data layout
There was a problem hiding this comment.
The protocol schema check is great. But is it possible that there could be a false false-positive (author decides a change of schema doesn't alter the data layout or it's backward compatible, but they are wrong)? When the checks fail, it's still up to developers to decide whether migration is needed. Cows here are alright, the serialization is straightforward, kinda obvious it wouldn't change the serialized format and it's easy to verify. But for more intricate cases, is there a safety net to ensure the data layout actually doesn't change? I may be overthinking. :))
There was a problem hiding this comment.
It's possible but in my experience it's more likely that a developer forgets to add ProtocolSchema to a new structure.
We have canary nodes running on master (updated nightly) so if there is any protocol upgrade that isn't correctly handled we would see that. Then we have pre-release tests where we spin up a small network and gradually update nodes. Finally there is testnet and we really shouldn't break anything there but it is the final line of defense before mainnet.
In that case, as this is not critical, it could wait until your PR is landed. :) |
Addressing #12313 (comment) from @wacban
Getting rid of the
Cows should make the receipt interface nicer to use.