Feature/nullification costing #1949
Conversation
|
Docker tags |
Benchmark for 32ffeceClick to view benchmark
|
| .and_then(|_| { | ||
| match hash_nullification { | ||
| IntentHashNullification::TransactionIntent { .. } => { | ||
| // Transaction intent nullification is historically not costed. |
There was a problem hiding this comment.
Can we clarify that it's nominally included in the base fee?
There was a problem hiding this comment.
There is no base fee, but tx_payload_cost fee which is proportional to payload size.
// The size of a typical transfer transaction is 400 bytes, and the cost will be 400 * 40 = 16,000 cost units
// The max size of a transaction is 1 MiB, and the cost will be 1,048,576 * 40 = 41,943,040 cost units
// This is roughly 1/24 of storing data in substate store per current setup.
mul(cast(size), 40)
There was a problem hiding this comment.
Oh! I thought there was a base fee. Interesting. Perhaps we should add one at Cuttlefish to account for the ident? Or just bill for TransactionIntent nullification from Cuttlefish? (switching on the system logic version)
There was a problem hiding this comment.
We have various uncosted substate reads (such as systemboot, vmboot, kernelboot).
Per the comment above, the minimum transaction cost units is about 16,000, which is just enough to cover one ReadFromDBNotFound (which is much more expensive than ReadFromDB).
I think we can live with this for now, OR introduce a larger base.
There was a problem hiding this comment.
What do you think about the system logic tweak to also bill for transaction intent reads? Would be quite straightforward I imagine?
There was a problem hiding this comment.
Perhaps we should add one at Cuttlefish to account for the ident
At execution time, we can't tell if it's v1 or v2.
If we start charging for transaction intent from cuttlefish, fees for all transaction will be increased, which has a user impact. Not too sure about if it's worth to do it now.
There was a problem hiding this comment.
We're in system here, right?
We have access to the version number either from version state (current) or from a generic (if we merge #1919 ).
If we start charging for transaction intent from cuttlefish, fees for all transaction will be increased, which has a user impact. Not too sure about if it's worth to do it now.
It will, but not by much I imagine?
There was a problem hiding this comment.
We can expose such information to the system.
It's more about "if it's good to have a discrepancy between v1 and v2".
There was a problem hiding this comment.
It will, but not by much I imagine?
It's about 16,000.
| @@ -290,6 +295,9 @@ pub mod owned { | |||
| RefCheck { | |||
| event: RefCheckEventOwned, | |||
| }, | |||
| Nullification { | |||
| io_access: Vec<IOAccess>, | |||
There was a problem hiding this comment.
Naively, I'd have expected this to be split between execution cost for the read, and finalization cost for the write?
There was a problem hiding this comment.
Also I wonder if we should use more intuitive names here, for the purpose of the receipt? (because "Nullification" is not really a common name in crypto/literature, even if we're using it internally)
ExecutionCostingEntry::CheckIntentValidityFinalizationCostingEntry::RecordIntentCommits
There was a problem hiding this comment.
Sounds reasonable to me.
There was a problem hiding this comment.
FinalizationCostingEntry::RecordIntentCommits is actually covered by FinalizationCostingEntry::CommitStateUpdates
There was a problem hiding this comment.
Are you sure? When I looked at the code, I thought I saw that finalisation of the state updates (i.e. CommitStateUpdates) was calculated from the state changes before the intent state changes were added (which happens at receipt creation time after finalization, because it needs to be after the result is decided), so I don’t believe the cost of the state updates was captured. This is why I suggested separating them explicitly
There was a problem hiding this comment.
You're right! I don't know what I was thinking. Let me add it explicitly.
And looks like we don't need to defer this cost, but apply at finalization time. If it fails, there is no commit anyway. Even fairer to users.
There was a problem hiding this comment.
Sounds good.
And if we (ever) add billing for non-contingent intents (currently just transaction intents), these should possibly be billed up front/deferred.
iamyulong
force-pushed
the
feature/nullification-costing
branch
from
bc95310 to
31bd7d9
Compare
iamyulong
force-pushed
the
feature/nullification-costing
branch
from
42ddd8b to
ef4aa5e
Compare
|
Merging this PR. The only pending question is "if we want to apply costing for the transaction intent hash status, and whether it's for v2 only OR v1 + v2", which can be addressed separately. |
Summary
Add deferred costing for nullifications.