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

monero: Use fee priority enums from monero repo CLI/RPC wallets #499

Merged
merged 3 commits into from
Feb 20, 2024

Conversation

j-berman
Copy link
Contributor

While testing fee calculation on mainnet, I noticed I was missing the highest priority level and that the CLI (and RPC) used different enums. Copied the enums from the CLI/RPC wallets.

For lack of a better place to include this... I also ran a manual test to make sure monero-serai has fee parity with wallet2 using the following script:

use rand_core::OsRng;

use monero_serai::wallet::{
  ViewPair, Scanner, SpendableOutput, Decoys,
  address::SubaddressIndex,
  SignableTransactionBuilder, Change, FeePriority,
  address::{Network, AddressSpec},
};

use zeroize::Zeroizing;

use std_shims::collections::HashSet;

use curve25519_dalek::{Scalar, edwards::CompressedEdwardsY};

mod runner;

fn offset(ring: &[u64]) -> Vec<u64> {
  let mut res = vec![ring[0]];
  res.resize(ring.len(), 0);
  for m in (1 .. ring.len()).rev() {
    res[m] = ring[m] - ring[m - 1];
  }
  res
}

async_sequential!(
  async fn mainnet_fees() {
    // CONSTANTS
    let tx_hash_bytes =
      hex::decode("TEST")
        .unwrap()
        .try_into()
        .unwrap();
    let spend_pub_bytes =
      hex::decode("TEST")
        .unwrap()
        .try_into()
        .unwrap();
    let view_priv_bytes =
      hex::decode("TEST")
        .unwrap()
        .try_into()
        .unwrap();

    let num_inputs = 1;
    
    // First use the CLI or RPC wallet to try to transfer a single output, but don't submit.
    // Take note of the fee and of the global output indexes in the ring in the constructed tx and manually include them in 
    // the candidates vec below. Obviously this can be automated but I just wanted a quick sanity check
    // against mainnet.
    // let candidates: Vec<u64> = Vec::from([]);
    // let spendable_global_index = 0;
    // END CONSTANTS

    let spend_pub = CompressedEdwardsY(spend_pub_bytes).decompress().unwrap();
    let view_priv = Zeroizing::new(Scalar::from_canonical_bytes(view_priv_bytes).unwrap());
    let view = ViewPair::new(spend_pub, view_priv);

    let mut scanner = Scanner::from_view(view.clone(), Some(HashSet::new()));

    let rpc = runner::rpc().await;

    let tx = rpc.get_transaction(tx_hash_bytes).await.unwrap();
    let output = scanner.scan_transaction(&tx).not_locked().swap_remove(0);
    let spendable_output = SpendableOutput::from(&rpc, output).await.unwrap();
    assert_eq!(spendable_global_index, spendable_output.global_index);

    let protocol = rpc.get_protocol().await.unwrap();
    let height = rpc.get_height().await.unwrap();

    // SET UP DECOYS
    let mut unlocked_decoys = Vec::with_capacity(protocol.ring_len());
    for (i, output) in
      rpc.get_unlocked_outputs(&candidates, height, false).await.unwrap().iter_mut().enumerate()
    {
      assert!(output.is_some());

      // Don't put the real in
      if candidates[i] == spendable_output.global_index {
        continue;
      }

      if let Some(output) = output.take() {
        unlocked_decoys.push((candidates[i], output));
      }
    }

    let o = (
      spendable_output.global_index,
      [spendable_output.key(), spendable_output.commitment().calculate()],
    );
    let mut decoys: Vec<Decoys> = Vec::with_capacity(num_inputs);
    let mut ring = unlocked_decoys
      .drain((unlocked_decoys.len() - (protocol.ring_len() - 1)) ..)
      .collect::<Vec<_>>();
    ring.push(o);
    ring.sort_by(|a, b| a.0.cmp(&b.0));
    decoys.push(Decoys {
      // Binary searches for the real spend since we don't know where it sorted to
      i: u8::try_from(ring.partition_point(|x| x.0 < o.0)).unwrap(),
      offsets: offset(&ring.iter().map(|output| output.0).collect::<Vec<_>>()),
      ring: ring.iter().map(|output| output.1).collect(),
    });
    // DONE setting up decoys

    for priority in [
      FeePriority::Default,
      FeePriority::Unimportant,
      FeePriority::Normal,
      FeePriority::Elevated,
      FeePriority::Priority,
    ] {
      let mut builder = SignableTransactionBuilder::new(
        protocol,
        rpc.get_fee(protocol, priority).await.unwrap(),
        Some(Change::new(&view, false)),
      );

      builder.add_input((spendable_output.clone(), decoys[0].clone()));

      let addr = view.address(Network::Mainnet, AddressSpec::Standard);
      let amount = spendable_output.commitment().amount * 9 / 10;
      builder.add_payment(addr, amount);

      let tx = builder.build().unwrap();
      dbg!(priority, tx.fee()); // make sure this matches up with the CLI's constructed txs
    }
  }
);

@kayabaNerve
Copy link
Member

kayabaNerve commented Jan 17, 2024

What's the rationale for this over wallet2's? The fact that this is what the monerod RPC is expecting?

Also, why is Default the lowest priority? Shouldn't the IRL Default be Normal?

@j-berman
Copy link
Contributor Author

wallet2.cpp doesn't use enums for the priority, it uses a uint32_t and brings priority > 4 down to 4: https://github.com/monero-project/monero/blob/059028a30a8ae9752338a7897329fe8012a310d5/src/wallet/wallet2.cpp#L8002

enums are more convenient and clearer for devs I figure, and the ones in this PR match the terms the CLI/RPC wallets use (CLI, RPC).

Before this PR I used my own enums loosely based off what the wallet2_api.h used, which is what the GUI/other wallets use to interface with wallet2 (source: https://github.com/monero-project/monero/blob/059028a30a8ae9752338a7897329fe8012a310d5/src/wallet/api/wallet2_api.h#L81-L87)

Default is the lowest priority in CLI/RPC/GUI/wallet2

@kayabaNerve
Copy link
Member

Considering default is not only an English word which doesn't mean "lowest", yet also has meaning in Rust, shouldn't we pick a better name?

@j-berman
Copy link
Contributor Author

I think it's named "default" in the core repo instead of "lowest" because if a tx with "Default" priority is provided to CLI/RPC/wallet using core repo wallet API/GUI, then by default, those wallets adjust the priority up to 1 based on the backlog in the pool if necessary (adjust_priority, CLI/RPC/GUI/wallet API). Note that that wallet setting can be changed.

Also note I placed a TODO in monero-serai to mach the behavior of adjust_priority:

// TODO: Implement wallet2's adjust_priority which by default automatically uses a lower
// priority than provided depending on the backlog in the pool

Nit on my end: technically this comment^ isn't accurate since adjust_priority can technically only ever adjust the priority from 0 to 1, and won't ever adjust a priority down e.g. from 3 to 1.

Done in consultation with @j-berman.

The RPC/CLI/GUI almost always adjust up except barring very explicit commands,
hence why FeePriority 0 is now only exposed via the explicit command of
FeePriority::Custom { priority: 0 }.

Also helps with terminology.
@kayabaNerve kayabaNerve merged commit cda14ac into serai-dex:develop Feb 20, 2024
18 checks passed
kayabaNerve added a commit that referenced this pull request Feb 25, 2024
* monero: Use fee priority enums from monero repo CLI/RPC wallets

* Update processor for fee priority change

* Remove FeePriority::Default

Done in consultation with @j-berman.

The RPC/CLI/GUI almost always adjust up except barring very explicit commands,
hence why FeePriority 0 is now only exposed via the explicit command of
FeePriority::Custom { priority: 0 }.

Also helps with terminology.

---------

Co-authored-by: Luke Parker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants