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

Jump to bottom

Add GM for base asset ID #700

Merged
merged 9 commits into from
Mar 25, 2024
Merged

Add GM for base asset ID #700

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4eac596
Refactor fn metadata
Dentosal Mar 20, 2024
9acd5e2
Add GM_BASE_ASSET_ID
Dentosal Mar 20, 2024
758e812
Add changelog entry
Dentosal Mar 20, 2024
013ba34
Clippy
Dentosal Mar 21, 2024
b78c16c
Fix typo bsae->base
Dentosal Mar 21, 2024
3fe06ca
Refactor fn metadata to use correct panics
Dentosal Mar 21, 2024
778d34f
Add a test case
Dentosal Mar 21, 2024
896fdae
Merge branch 'master' into dento/gm-base-asset-id
Dentosal Mar 23, 2024
6bf7acc
Merge branch 'master' into dento/gm-base-asset-id
Dentosal Mar 25, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
affected types.
- [#588](https://github.com/FuelLabs/fuel-vm/pull/588): Re-worked the size calculation of the canonical
serialization/deserialization.
- [#700](https://github.com/FuelLabs/fuel-vm/pull/700): Add `BASE_ASSET_ID` to `GM` instruction.


#### Removed

Expand Down
5 changes: 5 additions & 0 deletions fuel-asm/src/args.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ crate::enum_try_from! {

/// Get memory address where the transaction is located
TxStart = 0x05,

/// Get memory address of base asset ID
BaseAssetId = 0x06,
},
Immediate18
}
Expand Down Expand Up @@ -268,6 +271,8 @@ fn encode_gm_args() {
GMArgs::GetCaller,
GMArgs::GetVerifyingPredicate,
GMArgs::GetChainId,
GMArgs::TxStart,
GMArgs::BaseAssetId,
];

args.into_iter().for_each(|a| {
Expand Down
3 changes: 3 additions & 0 deletions fuel-asm/src/panic_reason.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@ enum_from! {
/// Attempt to use sequential memory instructions with too large slot count,
/// typically because it cannot fit into usize
TooManySlots = 0x2d,
/// Caller of this internal context is also expected to be internal,
/// i.e. $fp->$fp must be non-zero.
ExpectedNestedCaller = 0x2e,
}
}

Expand Down
7 changes: 4 additions & 3 deletions fuel-tx/src/transaction/consensus_parameters.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,9 +389,10 @@ impl TxParameters {
/// Transaction memory offset in VM runtime
pub const fn tx_offset(&self) -> usize {
Bytes32::LEN // Tx ID
+ WORD_SIZE // Tx size
// Asset ID/Balance coin input pairs
+ self.max_inputs as usize * (AssetId::LEN + WORD_SIZE)
+ AssetId::LEN // Base asset ID
// Asset ID/Balance coin input pairs
+ self.max_inputs as usize * (AssetId::LEN + WORD_SIZE)
+ WORD_SIZE // Tx size
Dentosal marked this conversation as resolved.
Show resolved Hide resolved
}

/// Replace the max inputs with the given argument
Expand Down
7 changes: 6 additions & 1 deletion fuel-vm/src/consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
//! VM parameters

use fuel_types::{
AssetId,
Bytes32,
Word,
};
Expand Down Expand Up @@ -41,7 +42,11 @@ static_assertions::const_assert!(VM_MAX_RAM < usize::MAX as u64);
// no limits to heap for now.

/// Offset for the assets balances in VM memory
pub const VM_MEMORY_BALANCES_OFFSET: usize = Bytes32::LEN;
pub const VM_MEMORY_BASE_ASSET_ID_OFFSET: usize = Bytes32::LEN;

/// Offset for the assets balances in VM memory
pub const VM_MEMORY_BALANCES_OFFSET: usize =
VM_MEMORY_BASE_ASSET_ID_OFFSET + AssetId::LEN;

/// Encoded len of a register id in an instruction (unused)
pub const VM_REGISTER_WIDTH: u8 = 6;
Expand Down
3 changes: 3 additions & 0 deletions fuel-vm/src/interpreter/initialization.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ where

self.push_stack(self.transaction().id(&self.chain_id()).as_ref())?;

let base_asset_id = self.interpreter_params.base_asset_id;
self.push_stack(&*base_asset_id)?;

runtime_balances.to_vm(self);

let tx_size = self.transaction().size() as Word;
Expand Down
69 changes: 22 additions & 47 deletions fuel-vm/src/interpreter/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,54 +108,29 @@ pub(crate) fn metadata(
chain_id: ChainId,
tx_offset: Word,
) -> SimpleResult<()> {
let external = context.is_external();
let args = GMArgs::try_from(imm)?;
let parent = context
.is_internal()
.then(|| frames.last().map(|f| f.registers()[RegId::FP]))
.flatten();

if external {
match args {
GMArgs::GetVerifyingPredicate => {
*result = context
.predicate()
.map(|p| p.idx() as Word)
.ok_or(PanicReason::TransactionValidity)?;
}

GMArgs::GetChainId => {
*result = chain_id.into();
}

GMArgs::TxStart => {
*result = tx_offset;
}

_ => return Err(PanicReason::ExpectedInternalContext.into()),
}
} else {
let parent = frames
.last()
.map(|f| f.registers()[RegId::FP])
.expect("External context will always have a frame");

match args {
GMArgs::IsCallerExternal => {
*result = (parent == 0) as Word;
}

GMArgs::GetCaller if parent != 0 => {
*result = parent;
}

GMArgs::GetChainId => {
*result = chain_id.into();
}

GMArgs::TxStart => {
*result = tx_offset;
}

_ => return Err(PanicReason::ExpectedInternalContext.into()),
}
}
*result = match GMArgs::try_from(imm)? {
GMArgs::GetVerifyingPredicate => context
Copy link
Collaborator

Choose a reason for hiding this comment

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

Before we were returning an ExpectedInternalContext error in the case of the internal context. Do we need to do the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. The new behavior is what the specs say. I see no reason to disallow internal contexts here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The specification says that it is allowed only in the context of the predicate:

image

And in our codebase predicate context is external context:

image

So it looks like we need ti check that it is external context=)

Copy link
Member Author

Choose a reason for hiding this comment

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

But the code already performs the check: The context.predicate() returns None outside predicate contexts, leading to TransactionValidity panic. Previously calling gm/GetVerifyingPredicate panicked with ExpectedInternalContext which was definitely incorrect. Now if we check with something like

if context.is_external() { // pseudocode
  result = context.predicate().map(...).ok_or(PanicReason::TransactionValidity)?;
} else {
  return PanicReason::ExpectedExternalContext;
}

then the only additional check we're actually performing is that Call contexts return ExpectedExternalContext instead of TransactionValidity. Is the issue just returning a wrong panic reason? Am I missing something here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ohh, I missed the moment that predicate already is doing this check inside=D Hmm, maybe using of another error will make more sense, because TransactionValidity is a strange choice

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to merge this as-is, since TransactionValidity is (only) used for this kind of things currently. We can do a rename in a separate PR if needed.

.predicate()
.map(|p| p.idx() as Word)
.ok_or(PanicReason::TransactionValidity)?,
GMArgs::GetChainId => chain_id.into(),
GMArgs::BaseAssetId => VM_MEMORY_BASE_ASSET_ID_OFFSET as Word,
GMArgs::TxStart => tx_offset,
GMArgs::GetCaller => match parent {
Some(0) => return Err(PanicReason::ExpectedNestedCaller.into()),
Some(parent) => parent,
None => return Err(PanicReason::ExpectedInternalContext.into()),
},
GMArgs::IsCallerExternal => match parent {
Some(p) => (p == 0) as Word,
None => return Err(PanicReason::ExpectedInternalContext.into()),
},
};

inc_pc(pc)?;
Ok(())
Expand Down
45 changes: 45 additions & 0 deletions fuel-vm/src/tests/metadata.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,51 @@ fn get_metadata_chain_id() {
}
}

#[test]
fn get_metadata_base_asset_id() {
Copy link
Member

@MitchTurner MitchTurner Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So, I'd say that the SUT of this test is op::gm_args and the condition is GMArgs::BaseAssetId. It's confusing since we can't call gm_args natively and instead we call transact, so you could also say that transact is the SUT in that sense.

Either way, I think the structure of the test should reflect what the SUT is. In this case I include it in a gm_args_script method, so it's a little indirect but cleans up the test in a good way IMO:

// given
let base_asset_id = AssetId::from([5; 32]);
let params = ConsensusParameters {
        base_asset_id,
        ..Default::default()
    };

// when
let arg = GMArgs::BaseAssetId;
let size = AssetId::LEN;
let script = gm_args_script(arg, size);

let logged_data = run_script_and_get_logged_data(params, script);

// then
let expected = params.base_asset_id.to_bytes();
let actual = logged_data;
assert_eq!(expected, actual);

So the name of the test could be something like gm_args__base_asset_id_retrieves_value_in_params

Copy link
Member Author

Choose a reason for hiding this comment

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

I think doing that would make the test case less readable. Note how this is already an integration test, not a unit test, and it still has minimal indirection which is quite nice. I experimented a bit with something like run_script_and_get_logged_data and going that way seems to lead essentially duplicating TransactionBuilder if we want to make it reusable.

let gas_limit = 1_000_000;
let height = BlockHeight::default();
let mut storage = MemoryStorage::default();

let mut params = ConsensusParameters::standard();
params.set_base_asset_id(AssetId::from([5; 32]));

let script = TransactionBuilder::script(
vec![
op::gm_args(0x20, GMArgs::BaseAssetId),
op::movi(0x21, AssetId::LEN.try_into().unwrap()),
op::logd(RegId::ZERO, RegId::ZERO, 0x20, 0x21),
op::ret(RegId::ONE),
]
.into_iter()
.collect(),
vec![],
)
.script_gas_limit(gas_limit)
.add_random_fee_input()
.finalize()
.into_checked(height, &params)
.unwrap();

let receipts = Transactor::<_, _>::new(
&mut storage,
InterpreterParams {
base_asset_id: *params.base_asset_id(),
..Default::default()
},
)
.transact(script)
.receipts()
.expect("Failed to transact")
.to_owned();

if let Receipt::LogData { data, .. } = receipts[0].clone() {
assert_eq!(data.unwrap(), params.base_asset_id().to_bytes());
} else {
panic!("expected LogData receipt, instead of {:?}", receipts[0]);
}
}

#[test]
fn get_metadata_tx_start() {
let gas_limit = 1_000_000;
Expand Down
7 changes: 2 additions & 5 deletions fuel-vm/src/tests/validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ use rand::{
#[cfg(feature = "alloc")]
use alloc::vec;

use crate::consts::WORD_SIZE;

#[test]
fn transaction_can_be_executed_after_maturity() {
const MATURITY: BlockHeight = BlockHeight::new(1);
Expand Down Expand Up @@ -62,9 +60,8 @@ fn malleable_fields_do_not_affect_validity() {

let params = ConsensusParameters::default();

let tx_size_ptr =
32 + (params.tx_params().max_inputs as usize * (AssetId::LEN + WORD_SIZE));
let tx_start_ptr = tx_size_ptr + 8;
let tx_start_ptr = params.tx_params().tx_offset();
let tx_size_ptr = tx_start_ptr - 8;

let tx = TransactionBuilder::script(
vec![
Expand Down
Loading