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.

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

[framework] Fix transaction fee deduct bug #2286

Merged
merged 2 commits into from
Jul 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 2 additions & 4 deletions crates/rooch-framework-tests/src/tests/bitcoin_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,8 @@ fn test_submit_block() {
bitcoin_module.get_block_by_height(height).unwrap().unwrap(),
block_header
);
assert_eq!(
bitcoin_module.get_latest_block_height().unwrap().unwrap(),
height
);
let latest_block = bitcoin_module.get_latest_block().unwrap().unwrap();
assert_eq!(height, latest_block.block_height,);
info!("txdata len: {}", bitcoin_txdata.len());

check_utxo(bitcoin_txdata, &binding_test);
Expand Down
38 changes: 19 additions & 19 deletions crates/rooch-framework-tests/src/tests/ord_test.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
// Copyright (c) RoochNetwork
// SPDX-License-Identifier: Apache-2.0

use crate::binding_test;
use bitcoin::consensus::Decodable;
use hex::FromHex;
use moveos_types::module_binding::MoveFunctionCaller;
use tracing::debug;

fn decode_inscription(btx_tx_hex: &str) {
let binding_test = binding_test::RustBindingTest::new().unwrap();
let btc_tx_bytes = Vec::from_hex(btx_tx_hex).unwrap();
let btc_tx: bitcoin::Transaction =
Decodable::consensus_decode(&mut btc_tx_bytes.as_slice()).unwrap();
Expand All @@ -24,24 +21,27 @@ fn decode_inscription(btx_tx_hex: &str) {
output.script_pubkey.p2wpkh_script_code()
);
}
let inscriptions = bitcoin_move::natives::ord::from_transaction(&btc_tx);
let _inscriptions = bitcoin_move::natives::ord::from_transaction(&btc_tx);

let ord_module = binding_test.as_module_binding::<rooch_types::bitcoin::ord::OrdModule>();
let move_btc_tx: rooch_types::bitcoin::types::Transaction =
rooch_types::bitcoin::types::Transaction::from(btc_tx);
let inscriptions_from_move = ord_module.from_transaction(&move_btc_tx).unwrap();
//TODO fixme the from_transaction function is not a read only function now.
//let binding_test = binding_test::RustBindingTest::new().unwrap();
// let ord_module = binding_test.as_module_binding::<rooch_types::bitcoin::ord::OrdModule>();
// let move_btc_tx: rooch_types::bitcoin::types::Transaction =
// rooch_types::bitcoin::types::Transaction::from(btc_tx);

for (i, (inscription, inscription_from_move)) in inscriptions
.into_iter()
.zip(inscriptions_from_move)
.enumerate()
{
debug!("{}. inscription: {:?}", i, inscription);
assert_eq!(
inscription.payload.body.unwrap_or_default(),
inscription_from_move.body
);
}
// let inscriptions_from_move = ord_module.from_transaction(&move_btc_tx).unwrap();

// for (i, (inscription, inscription_from_move)) in inscriptions
// .into_iter()
// .zip(inscriptions_from_move)
// .enumerate()
// {
// debug!("{}. inscription: {:?}", i, inscription);
// assert_eq!(
// inscription.payload.body.unwrap_or_default(),
// inscription_from_move.body
// );
// }
}

#[test]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ script {
}

//Get gas from faucet
//# run rooch_framework::gas_coin::faucet_entry --signers test --args u256:10000000000
//# run rooch_framework::gas_coin::faucet_entry --signers test --args u256:100000000000

//Transfer via coin store
//# run --signers test --args object:0xd073508b9582eff4e01078dc2e62489c15bbef91b6a2e568ac8fb33f0cf54daa
Expand Down
4 changes: 1 addition & 3 deletions crates/rooch-integration-test-runner/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,7 @@ impl<'a> MoveOSTestAdapter<'a> for MoveOSTestRunner<'a> {
genesis_gas_parameter.all_natives(),
MoveOSConfig::default(),
rooch_types::framework::system_pre_execute_functions(),
vec![],
//TODO FIXME https://github.com/rooch-network/rooch/issues/1137
//rooch_types::framework::system_post_execute_functions(),
rooch_types::framework::system_post_execute_functions(),
)
.unwrap();

Expand Down
28 changes: 13 additions & 15 deletions crates/rooch-types/src/bitcoin/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,7 @@ impl<'a> BitcoinModule<'a> {
pub const GET_BLOCK_BY_HEIGHT_FUNCTION_NAME: &'static IdentStr =
ident_str!("get_block_by_height");
pub const GET_BLOCK_HEIGHT_FUNCTION_NAME: &'static IdentStr = ident_str!("get_block_height");
pub const GET_LATEST_BLOCK_HEIGHT_FUNCTION_NAME: &'static IdentStr =
ident_str!("get_latest_block_height");
pub const GET_LATEST_BLOCK_FUNCTION_NAME: &'static IdentStr = ident_str!("get_latest_block");
pub const GET_UTXO_FUNCTION_NAME: &'static IdentStr = ident_str!("get_utxo");
pub const EXECUTE_L1_BLOCK_FUNCTION_NAME: &'static IdentStr = ident_str!("execute_l1_block");
pub const GET_GENESIS_BLOCK_FUNCTION_NAME: &'static IdentStr = ident_str!("get_genesis_block");
Expand Down Expand Up @@ -151,20 +150,19 @@ impl<'a> BitcoinModule<'a> {
Ok(height.into())
}

pub fn get_latest_block_height(&self) -> Result<Option<u64>> {
let call =
Self::create_function_call(Self::GET_LATEST_BLOCK_HEIGHT_FUNCTION_NAME, vec![], vec![]);
pub fn get_latest_block(&self) -> Result<Option<BlockHeightHash>> {
let call = Self::create_function_call(Self::GET_LATEST_BLOCK_FUNCTION_NAME, vec![], vec![]);
let ctx = TxContext::new_readonly_ctx(AccountAddress::ZERO);
let height = self
.caller
.call_function(&ctx, call)?
.into_result()
.map(|mut values| {
let value = values.pop().expect("should have one return value");
bcs::from_bytes::<MoveOption<u64>>(&value.value)
.expect("should be a valid MoveOption<u64>")
})?;
Ok(height.into())
let height_hash =
self.caller
.call_function(&ctx, call)?
.into_result()
.map(|mut values| {
let value = values.pop().expect("should have one return value");
bcs::from_bytes::<MoveOption<BlockHeightHash>>(&value.value)
.expect("should be a valid MoveOption<BlockHeightHash>")
})?;
Ok(height_hash.into())
}

pub fn get_genesis_block(&self) -> Result<BlockHeightHash> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,11 @@ The specific interfaces are as follows:

- `Option<Header>`: If the block exists, return the block header; otherwise, return none.

- `get_latest_block_height`: Get the height of the latest block.
- `get_latest_block`: Get the height and hash of the latest block.

- Return Value

- `Option<u64>`: If the latest block exists, return the block height; otherwise, return none.
- `Option<BlockHeightHash>`: If the latest block exists, return the block height; otherwise, return none.

### 2. `Header` Structure and Reading Interfaces

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,11 @@ Rooch 内置了 [bitcoin-move](https://github.com/rooch-network/rooch/tree/main/

- `Option<Header>`: 如果区块存在,则返回区块头,否则返回 none。

- `get_latest_block_height`: 于获取最新区块的高度
- `get_latest_block`: 于获取最新区块的高度以及 Hash

- 返回值

- `Option<u64>`: 如果存在最新区块,则返回区块高度,否则返回 none。
- `Option<BlockHeightHash>`: 如果存在最新区块,则返回区块高度和 Hash,否则返回 none。

### 2. `Header` 结构体与读取接口

Expand Down
12 changes: 12 additions & 0 deletions frameworks/rooch-framework/doc/transaction_fee.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
- [Function `genesis_init`](#0x3_transaction_fee_genesis_init)
- [Function `get_gas_factor`](#0x3_transaction_fee_get_gas_factor)
- [Function `calculate_gas`](#0x3_transaction_fee_calculate_gas)
- [Function `withdraw_fee`](#0x3_transaction_fee_withdraw_fee)
- [Function `deposit_fee`](#0x3_transaction_fee_deposit_fee)


Expand Down Expand Up @@ -65,6 +66,17 @@ Returns the gas factor of gas.



<a name="0x3_transaction_fee_withdraw_fee"></a>

## Function `withdraw_fee`



<pre><code><b>public</b>(<b>friend</b>) <b>fun</b> <a href="transaction_fee.md#0x3_transaction_fee_withdraw_fee">withdraw_fee</a>(amount: u256): <a href="coin.md#0x3_coin_Coin">coin::Coin</a>&lt;<a href="gas_coin.md#0x3_gas_coin_GasCoin">gas_coin::GasCoin</a>&gt;
</code></pre>



<a name="0x3_transaction_fee_deposit_fee"></a>

## Function `deposit_fee`
Expand Down
1 change: 1 addition & 0 deletions frameworks/rooch-framework/doc/transaction_validator.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
<b>use</b> <a href="">0x2::tx_result</a>;
<b>use</b> <a href="account.md#0x3_account">0x3::account</a>;
<b>use</b> <a href="account_authentication.md#0x3_account_authentication">0x3::account_authentication</a>;
<b>use</b> <a href="account_coin_store.md#0x3_account_coin_store">0x3::account_coin_store</a>;
<b>use</b> <a href="address_mapping.md#0x3_address_mapping">0x3::address_mapping</a>;
<b>use</b> <a href="auth_validator.md#0x3_auth_validator">0x3::auth_validator</a>;
<b>use</b> <a href="auth_validator_registry.md#0x3_auth_validator_registry">0x3::auth_validator_registry</a>;
Expand Down
7 changes: 7 additions & 0 deletions frameworks/rooch-framework/sources/transaction_fee.move
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ module rooch_framework::transaction_fee {
(gas_amount as u256) * (get_gas_factor() as u256)
}

public(friend) fun withdraw_fee(amount: u256) : Coin<GasCoin> {
let object_id = object::named_object_id<TransactionFeePool>();
let pool_object = object::borrow_mut_object_extend<TransactionFeePool>(object_id);
let pool = object::borrow_mut(pool_object);
coin_store::withdraw<GasCoin>(&mut pool.fee, amount)
}

public(friend) fun deposit_fee(gas_coin: Coin<GasCoin>) {
let object_id = object::named_object_id<TransactionFeePool>();
let pool_object = object::borrow_mut_object_extend<TransactionFeePool>(object_id);
Expand Down
23 changes: 18 additions & 5 deletions frameworks/rooch-framework/sources/transaction_validator.move
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ module rooch_framework::transaction_validator {
use rooch_framework::session_validator;
use rooch_framework::bitcoin_validator;
use rooch_framework::address_mapping;
use rooch_framework::account_coin_store;

const MAX_U64: u128 = 18446744073709551615;

Expand Down Expand Up @@ -120,8 +121,8 @@ module rooch_framework::transaction_validator {
account_entry::create_account(sender);
//if the chain is local or dev, give the sender some RGC
if (chain_id::is_local_or_dev()) {
//100 RGC
let init_gas = 1_00_000_000u256;
//10000 RGC
let init_gas = 1000_000_000_000u256;
gas_coin::faucet(sender, init_gas);
};
};
Expand All @@ -134,6 +135,11 @@ module rooch_framework::transaction_validator {
let module_signer = module_signer<TransactionValidatorPlaceholder>();
timestamp::try_update_global_time(&module_signer, tx_timestamp);
};
let gas_payment_account = tx_context::tx_gas_payment_account();
let max_gas_amount = tx_context::max_gas_amount();
let gas = transaction_fee::calculate_gas(max_gas_amount);
let gas_coin = gas_coin::deduct_gas(gas_payment_account, gas);
transaction_fee::deposit_fee(gas_coin);
}

/// Transaction post_execute function.
Expand All @@ -157,8 +163,15 @@ module rooch_framework::transaction_validator {
let tx_result = tx_context::tx_result();
let gas_payment_account = tx_context::tx_gas_payment_account();
let gas_used = tx_result::gas_used(&tx_result);
let gas = transaction_fee::calculate_gas(gas_used);
let gas_coin = gas_coin::deduct_gas(gas_payment_account, gas);
transaction_fee::deposit_fee(gas_coin);
let gas_used_after_scale = transaction_fee::calculate_gas(gas_used);

let max_gas_amount = tx_context::max_gas_amount();
let paid_gas = transaction_fee::calculate_gas(max_gas_amount);

if (gas_used_after_scale < paid_gas) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

实际支付的 gas,如果比预先扣除的 gas 要多怎么办?出现的情况是,网络变的拥挤之后 Gas 费上涨

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gas factor 调整会在交易执行之后,这个后面设计 gas factor 调整方案的时候需要考虑。 #1733

let refund_gas = paid_gas - gas_used_after_scale;
let refund_gas_coin = transaction_fee::withdraw_fee(refund_gas);
account_coin_store::deposit(gas_payment_account, refund_gas_coin);
};
}
}
30 changes: 27 additions & 3 deletions moveos/moveos-object-runtime/src/runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,17 @@ impl<'r> ObjectRuntimeContext<'r> {
}
}

pub(crate) enum RuntimeObjectArg {
Ref(Value),
Value(Value),
}

/// A structure representing mutable data of the ObjectRuntimeContext. This is in a RefCell
/// of the overall context so we can mutate while still accessing the overall context.
pub struct ObjectRuntime<'r> {
pub(crate) tx_context: TxContextValue,
pub(crate) root: RuntimeObject,
pub(crate) object_pointer_in_args: BTreeMap<ObjectID, Value>,
pub(crate) object_pointer_in_args: BTreeMap<ObjectID, RuntimeObjectArg>,
resolver: &'r dyn StatelessResolver,
}

Expand Down Expand Up @@ -383,7 +388,7 @@ impl<'r> ObjectRuntime<'r> {
//We cache the object pointer value in the object_pointer_in_args
//Ensure the reference count and the object can not be borrowed in Move
self.object_pointer_in_args
.insert(object_id.clone(), pointer_value);
.insert(object_id.clone(), RuntimeObjectArg::Ref(pointer_value));
}
ObjectArg::Value(_obj) => {
let pointer_value = rt_obj
Expand All @@ -392,14 +397,33 @@ impl<'r> ObjectRuntime<'r> {
//We cache the object pointer value in the object_pointer_in_args
//Ensure the reference count and the object can not be borrowed in Move
self.object_pointer_in_args
.insert(object_id.clone(), pointer_value);
.insert(object_id.clone(), RuntimeObjectArg::Value(pointer_value));
}
}
}
}
Ok(())
}

/// We need to release the Object pointer reference after executing the user function
/// Because the system post function maybe need to access the object
pub fn release_arguments(&mut self) -> PartialVMResult<()> {
let object_pointer_in_args = std::mem::take(&mut self.object_pointer_in_args);
for (_object_id, obj_arg) in object_pointer_in_args {
match obj_arg {
RuntimeObjectArg::Ref(_pointer) => {
// Just drop the reference
}
RuntimeObjectArg::Value(_pointer) => {
// The object is taken out when resolving the arguments
// and it should already be handle(transfer or remove) in the Move code
// So we just drop the object
}
}
}
Ok(())
}

// into inner
pub fn into_inner(self) -> (TxContext, RuntimeObject) {
let ObjectRuntime {
Expand Down
17 changes: 12 additions & 5 deletions moveos/moveos/src/moveos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -543,11 +543,18 @@ impl MoveOS {
// update txn result to TxContext
let gas_used = session.query_gas_used();
let tx_result = TxResult::new(&kept_status, gas_used);
session
.object_runtime
.write()
.add_to_tx_context(tx_result)
.expect("Add tx_result to TxContext should always success");
{
let mut runtime = session.object_runtime.write();

runtime
.add_to_tx_context(tx_result)
.expect("Add tx_result to TxContext should always success");
//We need to release the arguments before the post_execute function.
//Because the post_execute function may use the Object in the argument.
runtime
.release_arguments()
.expect("release_arguments should always success");
}

// We do not execute post_execute function for system call
if !is_system_call {
Expand Down
3 changes: 2 additions & 1 deletion scripts/pr.sh
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ cargo build

if [ ! -z "$ALSO_TEST" ]; then
cargo nextest run --workspace --all-features --exclude rooch-framework-tests --exclude rooch-integration-test-runner -v
cargo test --release run -p rooch-framework-tests -p rooch-integration-test-runner
cargo test -p rooch-framework-tests -p rooch-integration-test-runner
cargo test --release -p rooch-framework-tests bitcoin_test
RUST_LOG=warn cargo test -p testsuite --test integration
fi

Expand Down
Loading