Skip to content

Commit

Permalink
Merge branch 'grarco/better-gas-interface' (#3428)
Browse files Browse the repository at this point in the history
* grarco/better-gas-interface:
  Logs tx used gas
  Increases masp gas limit in genesis files and removes custom setup in integration tests
  update Hermes
  Returns `WholeGas` from `dry_run_tx`
  Adds conversion from `WholeGas` to `GasLimit`
  Updates gas in ibc e2e test
  Fixes gas payment in masp integration tests
  Updates gas limit in unit tests
  Changelog #3428
  Changelog #3428
  Removes misleading gas message
  Fixes ibc e2e test
  Fixes unit tests
  Clippy + fmt
  Compacts `BatchResults` into `TxResult`
  Remove gas from `TxResult`. Adjusts dry run result
  Reduces gas scale param in genesis
  Introduces `WholeGas` type
  • Loading branch information
brentstone committed Jul 10, 2024
2 parents a467029 + 209da0c commit d102a7e
Show file tree
Hide file tree
Showing 27 changed files with 339 additions and 338 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Improved the interface of the gas type. Removed the duplicated gas used from
events. ([\#3428](https://github.com/anoma/namada/pull/3428))
2 changes: 1 addition & 1 deletion .github/workflows/scripts/hermes.txt
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1.9.0-namada-beta13-rc2
1.9.0-namada-beta14-rc
6 changes: 3 additions & 3 deletions crates/gas/src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@

use namada_events::extend::EventAttributeEntry;

use super::Gas;
use crate::WholeGas;

/// Extend an [`namada_events::Event`] with gas used data.
pub struct GasUsed(pub Gas);
pub struct GasUsed(pub WholeGas);

impl EventAttributeEntry<'static> for GasUsed {
type Value = Gas;
type Value = WholeGas;
type ValueOwned = Self::Value;

const KEY: &'static str = "gas_used";
Expand Down
63 changes: 45 additions & 18 deletions crates/gas/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,9 +116,10 @@ pub const MASP_PARALLEL_GAS_DIVIDER: u64 = PARALLEL_GAS_DIVIDER / 2;
/// Gas module result for functions that may fail
pub type Result<T> = std::result::Result<T, Error>;

/// Representation of gas in sub-units. This effectively decouples gas metering
/// from fee payment, allowing higher resolution when accounting for gas while,
/// at the same time, providing a contained gas value when paying fees.
/// Representation of tracking gas in sub-units. This effectively decouples gas
/// metering from fee payment, allowing higher resolution when accounting for
/// gas while, at the same time, providing a contained gas value when paying
/// fees.
#[derive(
Clone,
Copy,
Expand Down Expand Up @@ -154,8 +155,8 @@ impl Gas {
}

/// Converts the sub gas units to whole ones. If the sub units are not a
/// multiple of the `SCALE` than ceil the quotient
pub fn get_whole_gas_units(&self, scale: u64) -> u64 {
/// multiple of the scale than ceil the quotient
pub fn get_whole_gas_units(&self, scale: u64) -> WholeGas {
let quotient = self
.sub
.checked_div(scale)
Expand All @@ -166,18 +167,18 @@ impl Gas {
.expect("Gas quotient remainder should not overflow")
== 0
{
quotient
quotient.into()
} else {
quotient
.checked_add(1)
.expect("Cannot overflow as the quotient is scaled down u64")
.into()
}
}

/// Generates a `Gas` instance from a whole amount
pub fn from_whole_units(whole: u64, scale: u64) -> Option<Self> {
let sub = whole.checked_mul(scale)?;
Some(Self { sub })
/// Generates a `Gas` instance from a `WholeGas` amount
pub fn from_whole_units(whole: WholeGas, scale: u64) -> Option<Self> {
scale.checked_mul(whole.into()).map(|sub| Self { sub })
}
}

Expand All @@ -193,20 +194,46 @@ impl From<Gas> for u64 {
}
}

impl Display for Gas {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
// Need to do this now that the gas scale is a parameter. Should
// manually scale gas first before calling this
write!(f, "{}", self.sub)
/// Gas represented in whole units. Used for fee payment and to display
/// information to the user.
#[derive(
Debug,
Clone,
Copy,
PartialEq,
BorshSerialize,
BorshDeserialize,
BorshDeserializer,
BorshSchema,
Serialize,
Deserialize,
Eq,
)]
pub struct WholeGas(u64);

impl From<u64> for WholeGas {
fn from(amount: u64) -> WholeGas {
Self(amount)
}
}

impl FromStr for Gas {
impl From<WholeGas> for u64 {
fn from(whole: WholeGas) -> u64 {
whole.0
}
}

impl FromStr for WholeGas {
type Err = GasParseError;

fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
let raw: u64 = s.parse().map_err(GasParseError::Parse)?;
Ok(Self { sub: raw })
Ok(Self(s.parse().map_err(GasParseError::Parse)?))
}
}

impl Display for WholeGas {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", self.0)
}
}

Expand Down
4 changes: 2 additions & 2 deletions crates/light_sdk/src/reading/asynchronous/tx.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use namada_sdk::events::Event;
use namada_sdk::rpc::{TxEventQuery, TxResponse};
use namada_sdk::tx::data::TxResult;
use namada_sdk::tx::data::DryRunResult;

use super::*;

Expand All @@ -25,7 +25,7 @@ pub async fn query_tx_events(
pub async fn dry_run_tx(
tendermint_addr: &str,
tx_bytes: Vec<u8>,
) -> Result<TxResult<String>, Error> {
) -> Result<DryRunResult, Error> {
let client = HttpClient::new(
TendermintAddress::from_str(tendermint_addr)
.map_err(|e| Error::Other(e.to_string()))?,
Expand Down
2 changes: 1 addition & 1 deletion crates/light_sdk/src/reading/blocking/tx.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub fn query_tx_events(
pub fn dry_run_tx(
tendermint_addr: &str,
tx_bytes: Vec<u8>,
) -> Result<TxResult, Error> {
) -> Result<DryRunResult, Error> {
let client = HttpClient::new(
TendermintAddress::from_str(tendermint_addr)
.map_err(|e| Error::Other(e.to_string()))?,
Expand Down
16 changes: 10 additions & 6 deletions crates/namada/src/ledger/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ mod dry_run_tx {

use namada_sdk::queries::{EncodedResponseQuery, RequestCtx, RequestQuery};
use namada_state::{DBIter, ResultExt, StorageHasher, DB};
use namada_tx::data::{ExtendedTxResult, GasLimit, TxResult};
use namada_tx::data::{DryRunResult, ExtendedTxResult, GasLimit, TxResult};

use super::protocol;
use crate::vm::wasm::{TxCache, VpCache};
Expand Down Expand Up @@ -132,18 +132,22 @@ mod dry_run_tx {
} else {
temp_state.write_log_mut().drop_tx();
}
tx_result.batch_results.insert_inner_tx_result(
tx_result.insert_inner_tx_result(
wrapper_hash.as_ref(),
either::Right(cmt),
batched_tx_result,
);
}
// Account gas for both batch and wrapper
tx_result.gas_used = tx_gas_meter.borrow().get_tx_consumed_gas();
let gas_used = tx_gas_meter
.borrow()
.get_tx_consumed_gas()
.get_whole_gas_units(gas_scale);
let tx_result_string = tx_result.to_result_string();
let data = tx_result_string.serialize_to_vec();
let dry_run_result = DryRunResult(tx_result_string, gas_used);

Ok(EncodedResponseQuery {
data,
data: dry_run_result.serialize_to_vec(),
proof: None,
info: Default::default(),
height: ctx.state.in_mem().get_last_block_height(),
Expand Down Expand Up @@ -331,7 +335,7 @@ mod test {
assert!(
result
.data
.batch_results
.0
.get_inner_tx_result(None, either::Right(cmt))
.unwrap()
.as_ref()
Expand Down
84 changes: 32 additions & 52 deletions crates/namada/src/ledger/protocol/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ use namada_token::utils::is_masp_transfer;
use namada_tx::action::Read;
use namada_tx::data::protocol::{ProtocolTx, ProtocolTxType};
use namada_tx::data::{
BatchResults, BatchedTxResult, ExtendedTxResult, TxResult, VpStatusFlags,
VpsResult, WrapperTx,
BatchedTxResult, ExtendedTxResult, TxResult, VpStatusFlags, VpsResult,
WrapperTx,
};
use namada_tx::{BatchedTxRef, Tx, TxCommitments};
use namada_vote_ext::EthereumTxData;
Expand Down Expand Up @@ -275,17 +275,14 @@ where
},
)?;

Ok(TxResult {
gas_used: tx_gas_meter.borrow().get_tx_consumed_gas(),
batch_results: {
let mut batch_results = BatchResults::new();
batch_results.insert_inner_tx_result(
wrapper_hash,
either::Right(cmt),
Ok(batched_tx_result),
);
batch_results
},
Ok({
let mut batch_results = TxResult::new();
batch_results.insert_inner_tx_result(
wrapper_hash,
either::Right(cmt),
Ok(batched_tx_result),
);
batch_results
}
.to_extended_result(None))
}
Expand All @@ -296,17 +293,14 @@ where
let batched_tx_result =
apply_protocol_tx(protocol_tx.tx, tx.data(cmt), state)?;

Ok(TxResult {
batch_results: {
let mut batch_results = BatchResults::new();
batch_results.insert_inner_tx_result(
None,
either::Right(cmt),
Ok(batched_tx_result),
);
batch_results
},
..Default::default()
Ok({
let mut batch_results = TxResult::new();
batch_results.insert_inner_tx_result(
None,
either::Right(cmt),
Ok(batched_tx_result),
);
batch_results
}
.to_extended_result(None))
}
Expand Down Expand Up @@ -382,16 +376,11 @@ where
) {
Err(Error::GasError(ref msg)) => {
// Gas error aborts the execution of the entire batch
extended_tx_result.tx_result.gas_used =
tx_gas_meter.borrow().get_tx_consumed_gas();
extended_tx_result
.tx_result
.batch_results
.insert_inner_tx_result(
wrapper_hash,
either::Right(cmt),
Err(Error::GasError(msg.to_owned())),
);
extended_tx_result.tx_result.insert_inner_tx_result(
wrapper_hash,
either::Right(cmt),
Err(Error::GasError(msg.to_owned())),
);
state.write_log_mut().drop_tx();
return Err(DispatchError {
error: Error::GasError(msg.to_owned()),
Expand All @@ -402,16 +391,11 @@ where
let is_accepted =
matches!(&res, Ok(result) if result.is_accepted());

extended_tx_result
.tx_result
.batch_results
.insert_inner_tx_result(
wrapper_hash,
either::Right(cmt),
res,
);
extended_tx_result.tx_result.gas_used =
tx_gas_meter.borrow().get_tx_consumed_gas();
extended_tx_result.tx_result.insert_inner_tx_result(
wrapper_hash,
either::Right(cmt),
res,
);
if is_accepted {
// If the transaction was a masp one append the
// transaction refs for the events
Expand Down Expand Up @@ -488,9 +472,9 @@ where
shell_params.state.write_log_mut().commit_batch();

let (batch_results, masp_tx_refs) = payment_result.map_or_else(
|| (BatchResults::default(), None),
|| (TxResult::default(), None),
|(batched_result, masp_section_ref)| {
let mut batch = BatchResults::default();
let mut batch = TxResult::default();
batch.insert_inner_tx_result(
// Ok to unwrap cause if we have a batched result it means
// we've executed the first tx in the batch
Expand All @@ -508,11 +492,7 @@ where
.add_wrapper_gas(tx_bytes)
.map_err(|err| Error::GasError(err.to_string()))?;

Ok(TxResult {
gas_used: tx_gas_meter.borrow().get_tx_consumed_gas(),
batch_results,
}
.to_extended_result(masp_tx_refs))
Ok(batch_results.to_extended_result(masp_tx_refs))
}

/// Perform the actual transfer of fees from the fee payer to the block
Expand Down Expand Up @@ -702,7 +682,7 @@ where
let gas_scale = get_gas_scale(&**state).map_err(Error::StorageError)?;

let mut gas_meter = TxGasMeter::new(
namada_gas::Gas::from_whole_units(max_gas_limit, gas_scale)
namada_gas::Gas::from_whole_units(max_gas_limit.into(), gas_scale)
.ok_or_else(|| {
Error::GasError("Overflow in gas expansion".to_string())
})?,
Expand Down
35 changes: 14 additions & 21 deletions crates/node/src/bench_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,7 @@ use namada::masp::MaspTxRefs;
use namada::state::StorageRead;
use namada::token::{Amount, DenominatedAmount, Transfer};
use namada::tx::data::pos::Bond;
use namada::tx::data::{
BatchResults, BatchedTxResult, Fee, TxResult, VpsResult,
};
use namada::tx::data::{BatchedTxResult, Fee, TxResult, VpsResult};
use namada::tx::event::{new_tx_event, Batch};
use namada::tx::{
Authorization, BatchedTx, BatchedTxRef, Code, Data, Section, Tx,
Expand Down Expand Up @@ -919,24 +917,19 @@ impl Client for BenchShell {
.iter()
.enumerate()
.map(|(idx, (tx, changed_keys))| {
let tx_result = TxResult::<String> {
gas_used: 0.into(),
batch_results: {
let mut batch_results = BatchResults::new();
batch_results.insert_inner_tx_result(
tx.wrapper_hash().as_ref(),
either::Right(
tx.first_commitments().unwrap(),
),
Ok(BatchedTxResult {
changed_keys: changed_keys.to_owned(),
vps_result: VpsResult::default(),
initialized_accounts: vec![],
events: BTreeSet::default(),
}),
);
batch_results
},
let tx_result = {
let mut batch_results = TxResult::new();
batch_results.insert_inner_tx_result(
tx.wrapper_hash().as_ref(),
either::Right(tx.first_commitments().unwrap()),
Ok(BatchedTxResult {
changed_keys: changed_keys.to_owned(),
vps_result: VpsResult::default(),
initialized_accounts: vec![],
events: BTreeSet::default(),
}),
);
batch_results
};
let event: Event = new_tx_event(tx, height.value())
.with(Batch(&tx_result))
Expand Down
Loading

0 comments on commit d102a7e

Please sign in to comment.