Skip to content
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.

Sign up for GitHub

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

Feature/nullification costing #1949

Merged
merged 7 commits into from
Oct 8, 2024
Merged

Feature/nullification costing #1949

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6976fec
Defer nullification cost
iamyulong Oct 7, 2024
0a2bd44
Update scenarios
iamyulong Oct 7, 2024
eeb9a99
Merge remote-tracking branch 'origin/develop' into feature/nullificat…
iamyulong Oct 7, 2024
2b2aef3
Clean up code
iamyulong Oct 7, 2024
31bd7d9
Update assets
iamyulong Oct 7, 2024
ef4aa5e
Add FinalizationCost::CommitIntentStatus
iamyulong Oct 8, 2024
1f8f892
Update assets
iamyulong Oct 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 55 additions & 1 deletion radix-engine/src/system/system_callback.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1571,7 +1571,61 @@ impl<V: SystemCallbackObject> KernelTransactionExecutor for System<V> {
)
}
}
};
}
.and_then(|_| {
match hash_nullification {
IntentHashNullification::TransactionIntent { .. } => {
// Transaction intent nullification is historically not costed.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we clarify that it's nominally included in the base fee?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@dhedey dhedey Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@iamyulong iamyulong Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about the system logic tweak to also bill for transaction intent reads? Would be quite straightforward I imagine?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dhedey dhedey Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can expose such information to the system.

It's more about "if it's good to have a discrepancy between v1 and v2".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will, but not by much I imagine?

It's about 16,000.

}
IntentHashNullification::Subintent { .. } => {
if let Some(costing) = modules.costing_mut() {
let read_not_found =
iamyulong marked this conversation as resolved.
Show resolved Hide resolved
IOAccess::ReadFromDbNotFound(CanonicalSubstateKey {
node_id: TRANSACTION_TRACKER.into_node_id(),
partition_number: PartitionNumber(0),
iamyulong marked this conversation as resolved.
Show resolved Hide resolved
substate_key: SubstateKey::Map(
scrypto_encode(&Hash([0u8; 32])).unwrap(),
),
});
let write = IOAccess::TrackSubstateUpdated {
canonical_substate_key: CanonicalSubstateKey {
node_id: TRANSACTION_TRACKER.into_node_id(),
partition_number: PartitionNumber(0),
substate_key: SubstateKey::Map(
scrypto_encode(&Hash([0u8; 32])).unwrap(),
),
},
old_size: Some(0), // some are insertions but treated as updates, given the ring buffer design.
new_size: Some(
IndexedScryptoValue::from_typed(&KeyValueEntrySubstate::V1(
KeyValueEntrySubstateV1 {
value: Some(TransactionStatus::V1(
TransactionStatusV1::CommittedSuccess,
)),
lock_status: LockStatus::Unlocked,
},
))
.len(),
),
};
return costing
.apply_deferred_execution_cost(
iamyulong marked this conversation as resolved.
Show resolved Hide resolved
ExecutionCostingEntry::Nullification {
io_access: &[read_not_found, write],
},
)
.map_err(|e| {
RejectionReason::BootloadingError(
BootloadingError::FailedToApplyDeferredCosts(e),
)
});
}
}
}

Ok(())
});

match intent_hash_validation_result {
Ok(()) => {}
Err(error) => return Err(Self::create_rejection_receipt(error, modules)),
Expand Down
11 changes: 11 additions & 0 deletions radix-engine/src/system/system_modules/costing/costing_entry.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use crate::kernel::kernel_callback_api::{
use crate::system::actor::Actor;
use crate::system::system_modules::transaction_runtime::Event;
use crate::track::interface::StoreCommit;
use crate::track::IOAccess;

#[derive(Debug, IntoStaticStr)]
pub enum ExecutionCostingEntry<'a> {
Expand All @@ -21,6 +22,9 @@ pub enum ExecutionCostingEntry<'a> {
RefCheck {
event: &'a RefCheckEvent<'a>,
},
Nullification {
io_access: &'a [IOAccess],
},

/* run code */
RunNativeCode {
Expand Down Expand Up @@ -149,6 +153,7 @@ impl<'a> ExecutionCostingEntry<'a> {
} => ft.verify_tx_signatures_cost(*num_of_signatures),
ExecutionCostingEntry::ValidateTxPayload { size } => ft.validate_tx_payload_cost(*size),
ExecutionCostingEntry::RefCheck { event } => ft.ref_check(event),
ExecutionCostingEntry::Nullification { io_access } => ft.nullification(io_access),
ExecutionCostingEntry::RunNativeCode {
package_address,
export_name,
Expand Down Expand Up @@ -290,6 +295,9 @@ pub mod owned {
RefCheck {
event: RefCheckEventOwned,
},
Nullification {
io_access: Vec<IOAccess>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naively, I'd have expected this to be split between execution cost for the read, and finalization cost for the write?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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::CheckIntentValidity
  • FinalizationCostingEntry::RecordIntentCommits

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FinalizationCostingEntry::RecordIntentCommits is actually covered by FinalizationCostingEntry::CommitStateUpdates

Copy link
Contributor

@dhedey dhedey Oct 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@dhedey dhedey Oct 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

And if we (ever) add billing for non-contingent intents (currently just transaction intents), these should possibly be billed up front/deferred.

},

/* run code */
RunNativeCode {
Expand Down Expand Up @@ -528,6 +536,9 @@ pub mod owned {
ExecutionCostingEntry::RefCheck { event } => Self::RefCheck {
event: event.into(),
},
ExecutionCostingEntry::Nullification { io_access } => Self::Nullification {
io_access: io_access.to_vec(),
},
ExecutionCostingEntry::RunNativeCode {
package_address,
export_name,
Expand Down
9 changes: 9 additions & 0 deletions radix-engine/src/system/system_modules/costing/fee_table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,15 @@ impl FeeTable {
}
}

#[inline]
pub fn nullification(&self, io_access: &[IOAccess]) -> u32 {
let mut sum = 0u32;
for io in io_access {
sum = sum.saturating_add(self.io_access_cost(io));
}
sum
}

#[inline]
pub fn run_native_code_cost(
&self,
Expand Down
35 changes: 18 additions & 17 deletions ...narios/generated-examples/cuttlefish/basic_subintents/costings/002--trivial_subintent.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,30 +1,31 @@
Total Cost (XRD) , 0.14138479477, 100.0%
- Execution Cost (XRD) , 0.08473095, 59.9%
- Finalization Cost (XRD) , 0.0052508, 3.7%
- Storage Cost (XRD) , 0.05140304477, 36.4%
Total Cost (XRD) , 0.14938479477, 100.0%
- Execution Cost (XRD) , 0.09273095, 62.1%
- Finalization Cost (XRD) , 0.0052508, 3.5%
- Storage Cost (XRD) , 0.05140304477, 34.4%
- Tipping Cost (XRD) , 0, 0.0%
- Royalty Cost (XRD) , 0, 0.0%
Execution Cost Breakdown , 1694619, 100.0%
Execution Cost Breakdown , 1854619, 100.0%
- AfterInvoke , 24, 0.0%
- AllocateNodeId , 776, 0.0%
- BeforeInvoke , 600, 0.0%
- CloseSubstate , 10191, 0.6%
- CloseSubstate , 10191, 0.5%
- CreateNode , 6498, 0.4%
- DropNode , 11994, 0.7%
- DropNode , 11994, 0.6%
- EmitEvent , 556, 0.0%
- LockFee , 500, 0.0%
- OpenSubstate::GlobalAccount , 244472, 14.4%
- OpenSubstate::GlobalFungibleResourceManager , 122465, 7.2%
- OpenSubstate::GlobalPackage , 901313, 53.2%
- OpenSubstate::InternalFungibleVault , 85206, 5.0%
- OpenSubstate::InternalGenericComponent , 13089, 0.8%
- Nullification , 160000, 8.6%
- OpenSubstate::GlobalAccount , 244472, 13.2%
- OpenSubstate::GlobalFungibleResourceManager , 122465, 6.6%
- OpenSubstate::GlobalPackage , 901313, 48.6%
- OpenSubstate::InternalFungibleVault , 85206, 4.6%
- OpenSubstate::InternalGenericComponent , 13089, 0.7%
- PinNode , 96, 0.0%
- QueryActor , 1000, 0.1%
- ReadSubstate , 67775, 4.0%
- RefCheck , 40011, 2.4%
- RunNativeCode::Worktop_drop , 35836, 2.1%
- RunNativeCode::lock_fee , 116047, 6.8%
- ValidateTxPayload , 18560, 1.1%
- ReadSubstate , 67775, 3.7%
- RefCheck , 40011, 2.2%
- RunNativeCode::Worktop_drop , 35836, 1.9%
- RunNativeCode::lock_fee , 116047, 6.3%
- ValidateTxPayload , 18560, 1.0%
- VerifyTxSignatures , 14000, 0.8%
- WriteSubstate , 3610, 0.2%
Finalization Cost Breakdown , 105016, 100.0%
Expand Down
20 changes: 10 additions & 10 deletions ...narios/generated-examples/cuttlefish/basic_subintents/receipts/002--trivial_subintent.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
TRANSACTION STATUS: COMMITTED SUCCESS

TRANSACTION COST: 0.14138479477 XRD
├─ Network execution: 0.08473095 XRD, 1694619 execution cost units
TRANSACTION COST: 0.14938479477 XRD
├─ Network execution: 0.09273095 XRD, 1854619 execution cost units
├─ Network finalization: 0.0052508 XRD, 105016 finalization cost units
├─ Tip: 0 XRD
├─ Network Storage: 0.05140304477 XRD
Expand All @@ -16,15 +16,15 @@ EVENTS: 4
}
├─ Emitter: Method { node: internal_vault_sim1trj4jx6nhjzx3jshetvefjfkrys34c2f3uw5j5vzydyltn5s5rce95, module_id: Main }
Event: PayFeeEvent {
amount: Decimal("0.14138479477"),
amount: Decimal("0.14938479477"),
}
├─ Emitter: Method { node: internal_vault_sim1tpsesv77qvw782kknjks9g3x2msg8cc8ldshk28pkf6m6lkhun3sel, module_id: Main }
Event: DepositEvent {
amount: Decimal("0.070692397385"),
amount: Decimal("0.074692397385"),
}
└─ Emitter: Method { node: resource_sim1tknxxxxxxxxxradxrdxxxxxxxxx009923554798xxxxxxxxxakj8n3, module_id: Main }
Event: BurnFungibleResourceEvent {
amount: Decimal("0.070692397385"),
amount: Decimal("0.074692397385"),
}

STATE UPDATES: 4 entities
Expand All @@ -34,7 +34,7 @@ STATE UPDATES: 4 entities
Value: UNLOCKED ConsensusManagerValidatorRewardsFieldPayload::V1(
ValidatorRewardsSubstate {
proposer_rewards: {
0u8 => Decimal("0.2005427530975"),
0u8 => Decimal("0.2025427530975"),
},
rewards_vault: Vault(Own("internal_vault_sim1tpsesv77qvw782kknjks9g3x2msg8cc8ldshk28pkf6m6lkhun3sel")),
},
Expand Down Expand Up @@ -64,13 +64,13 @@ STATE UPDATES: 4 entities
└─ Partition(64): 1 change
└─ Set: Field(0)
Value: UNLOCKED FungibleVaultBalanceFieldPayload::V1(
LiquidFungibleResource(Decimal("9999.85861520523")),
LiquidFungibleResource(Decimal("9999.85061520523")),
)
└─ internal_vault_sim1tpsesv77qvw782kknjks9g3x2msg8cc8ldshk28pkf6m6lkhun3sel across 1 partitions
└─ Partition(64): 1 change
└─ Set: Field(0)
Value: UNLOCKED FungibleVaultBalanceFieldPayload::V1(
LiquidFungibleResource(Decimal("0.401085506195")),
LiquidFungibleResource(Decimal("0.405085506195")),
)

OUTPUTS: 2
Expand All @@ -80,9 +80,9 @@ OUTPUTS: 2
BALANCE CHANGES: 2
├─ Vault: internal_vault_sim1trj4jx6nhjzx3jshetvefjfkrys34c2f3uw5j5vzydyltn5s5rce95
ResAddr: resource_sim1tknxxxxxxxxxradxrdxxxxxxxxx009923554798xxxxxxxxxakj8n3
Change: -0.14138479477
Change: -0.14938479477
└─ Vault: internal_vault_sim1tpsesv77qvw782kknjks9g3x2msg8cc8ldshk28pkf6m6lkhun3sel
ResAddr: resource_sim1tknxxxxxxxxxradxrdxxxxxxxxx009923554798xxxxxxxxxakj8n3
Change: 0.070692397385
Change: 0.074692397385

NEW ENTITIES: 0
4 changes: 2 additions & 2 deletions ...transaction-scenarios/generated-examples/cuttlefish/basic_subintents/scenario_summary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ Name: basic_subintents

== SUMMARY HASHES ==
These Cuttlefish hashes are permitted to change only until the scenario is deployed to a permanent network, else it can cause divergence.
State changes: 3116f51e554b5004 (allowed to change if not deployed to any network)
Events : fcb5856819d6dafc (allowed to change if not deployed to any network)
State changes: e5f6ee3518dabe79 (allowed to change if not deployed to any network)
Events : 9a31f8bf572b7a6f (allowed to change if not deployed to any network)

== INTERESTING ADDRESSES ==
- parent_account: account_sim1c9efznpk7w0xwdav45jj944cj677hcqj9el3atswwfw4dt84z3pun5
Expand Down
Loading