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

fix(ckbtc): Fix a problem with retrieve_btc_min_amount setting #2435

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix(ckbtc): Fix a problem with retrieve_btc_min_amount setting #2435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fe0aacc
fix(ckbtc): Fix a problem with retrieve_btc_min_amount setting
ninegua Nov 5, 2024
06dcebe
Fix tests
ninegua Nov 5, 2024
aced920
Add a new test case
ninegua Nov 5, 2024
b1ad6db
Clippy
ninegua Nov 5, 2024
5848aed
Typo
ninegua Nov 5, 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
5 changes: 5 additions & 0 deletions rs/bitcoin/ckbtc/minter/src/dashboard.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,10 @@ pub fn build_metadata(s: &CkBtcMinterState) -> String {
<th>Min retrieve BTC amount</th>
<td>{}</td>
</tr>
<tr>
<th>Min retrieve BTC amount (fee based)</th>
<td>{}</td>
</tr>
<tr>
<th>Total BTC managed</th>
<td>{}</td>
Expand All @@ -315,6 +319,7 @@ pub fn build_metadata(s: &CkBtcMinterState) -> String {
.unwrap_or_else(|| "N/A".to_string()),
DisplayAmount(s.kyt_fee),
DisplayAmount(s.retrieve_btc_min_amount),
DisplayAmount(s.fee_based_retrieve_btc_min_amount),
DisplayAmount(get_total_btc_managed(s))
)
}
Expand Down
11 changes: 3 additions & 8 deletions rs/bitcoin/ckbtc/minter/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,19 +166,14 @@ async fn fetch_main_utxos(main_account: &Account, main_address: &BitcoinAddress)
/// Returns the minimum withdrawal amount based on the current median fee rate (in millisatoshi per byte).
/// The returned amount is in satoshi.
fn compute_min_withdrawal_amount(
btc_network: Network,
median_fee_rate_e3s: MillisatoshiPerByte,
min_withdrawal_amount: u64,
) -> u64 {
const PER_REQUEST_RBF_BOUND: u64 = 22_100;
const PER_REQUEST_VSIZE_BOUND: u64 = 221;
const PER_REQUEST_MINTER_FEE_BOUND: u64 = 305;
const PER_REQUEST_KYT_FEE: u64 = 2_000;
Comment on lines 172 to 175
Copy link
Member

Choose a reason for hiding this comment

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

understanding question: do you know how those numbers were computed? Are they part of some Bitcoin spec.? (maybe @THLO knows)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the calculation is done here.
Yes, the calculation is done based on a 28-node subnet; however, we assume a Bitcoin price of 10,000 XDR, that is, we are currently overcharging quite a bit.


let min_withdrawal_amount = match btc_network {
Network::Testnet | Network::Regtest => 10_000,
Network::Mainnet => 100_000,
};

let median_fee_rate = median_fee_rate_e3s / 1_000;
((PER_REQUEST_RBF_BOUND
+ PER_REQUEST_VSIZE_BOUND * median_fee_rate
Comment on lines 178 to 179
Copy link
Contributor

Choose a reason for hiding this comment

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

The current logic increments the fee in chunks of 50k satoshis. Do we want to keep it that way?
Before, it was 100k -> 150k -> 200k... and now it would be 50k -> 100k -> 150k.

I think we can leave it like that. What do you think?

Expand Down Expand Up @@ -206,8 +201,8 @@ pub async fn estimate_fee_per_vbyte() -> Option<MillisatoshiPerByte> {
if fees.len() >= 100 {
state::mutate_state(|s| {
s.last_fee_per_vbyte.clone_from(&fees);
Copy link
Member

Choose a reason for hiding this comment

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

Here for a lack of a better place: The method estimate_fee_per_vbyte is called as part of 2 tasks:

  1. ProcessLogic
  2. RefreshFeePercentiles

The 1st one is guarded, ensuring that no concurrent execution between 2 ProcessLogic tasks can occur, but the 2nd is not, meaning that it could run concurrently to a ProcessLogic task (RefreshPercentiles are only scheduled upon canister initialization or upgrade and every hour, so that concurrent execution between 2 RefreshPercentiles tasks does not seem possible). So should we maybe guard the execution of estimate_fee_per_vbyte?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable to me. Paul, what do you think?

s.retrieve_btc_min_amount =
compute_min_withdrawal_amount(s.btc_network, fees[50]);
s.fee_based_retrieve_btc_min_amount =
compute_min_withdrawal_amount(fees[50], s.retrieve_btc_min_amount);
});
Some(fees[50])
} else {
Expand Down
2 changes: 1 addition & 1 deletion rs/bitcoin/ckbtc/minter/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ fn get_minter_info() -> MinterInfo {
read_state(|s| MinterInfo {
kyt_fee: s.kyt_fee,
min_confirmations: s.min_confirmations,
retrieve_btc_min_amount: s.retrieve_btc_min_amount,
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe some comment on the Candid file mentioning that this amount varies according to the current network fees may be helpful

retrieve_btc_min_amount: s.fee_based_retrieve_btc_min_amount,
})
}

Expand Down
6 changes: 6 additions & 0 deletions rs/bitcoin/ckbtc/minter/src/metrics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,12 @@ pub fn encode_metrics(
"Minimum number of ckBTC a user can withdraw.",
)?;

metrics.encode_gauge(
Copy link
Member

Choose a reason for hiding this comment

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

good idea to have a metric here!

"ckbtc_minter_fee_based_min_retrievable_amount",
state::read_state(|s| s.fee_based_retrieve_btc_min_amount) as f64,
"Minimum number of ckBTC a user can withdraw (fee based).",
)?;

metrics.encode_gauge(
"ckbtc_minter_min_confirmations",
state::read_state(|s| s.min_confirmations) as f64,
Expand Down
6 changes: 6 additions & 0 deletions rs/bitcoin/ckbtc/minter/src/state.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,9 @@ pub struct CkBtcMinterState {
/// Minimum amount of bitcoin that can be retrieved
pub retrieve_btc_min_amount: u64,

/// Minimum amount of bitcoin that can be retrieved based on recent fees
pub fee_based_retrieve_btc_min_amount: u64,

/// Retrieve_btc requests that are waiting to be served, sorted by
/// received_at.
pub pending_retrieve_btc_requests: Vec<RetrieveBtcRequest>,
Expand Down Expand Up @@ -432,6 +435,7 @@ impl CkBtcMinterState {
self.btc_network = btc_network.into();
self.ecdsa_key_name = ecdsa_key_name;
self.retrieve_btc_min_amount = retrieve_btc_min_amount;
self.fee_based_retrieve_btc_min_amount = retrieve_btc_min_amount;
self.ledger_id = ledger_id;
self.max_time_in_queue_nanos = max_time_in_queue_nanos;
self.mode = mode;
Expand All @@ -457,6 +461,7 @@ impl CkBtcMinterState {
) {
if let Some(retrieve_btc_min_amount) = retrieve_btc_min_amount {
self.retrieve_btc_min_amount = retrieve_btc_min_amount;
self.fee_based_retrieve_btc_min_amount = retrieve_btc_min_amount;
}
if let Some(max_time_in_queue_nanos) = max_time_in_queue_nanos {
self.max_time_in_queue_nanos = max_time_in_queue_nanos;
Expand Down Expand Up @@ -1236,6 +1241,7 @@ impl From<InitArgs> for CkBtcMinterState {
update_balance_principals: Default::default(),
retrieve_btc_principals: Default::default(),
retrieve_btc_min_amount: args.retrieve_btc_min_amount,
fee_based_retrieve_btc_min_amount: args.retrieve_btc_min_amount,
pending_retrieve_btc_requests: Default::default(),
requests_in_flight: Default::default(),
last_transaction_submission_time_ns: None,
Expand Down
18 changes: 14 additions & 4 deletions rs/bitcoin/ckbtc/minter/src/updates/retrieve_btc.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,13 @@ pub async fn retrieve_btc(args: RetrieveBtcArgs) -> Result<RetrieveBtcOk, Retrie
}

let _guard = retrieve_btc_guard(caller)?;
let (min_retrieve_amount, btc_network, kyt_fee) =
read_state(|s| (s.retrieve_btc_min_amount, s.btc_network, s.kyt_fee));
let (min_retrieve_amount, btc_network, kyt_fee) = read_state(|s| {
(
s.fee_based_retrieve_btc_min_amount,
s.btc_network,
s.kyt_fee,
)
});

let min_amount = max(min_retrieve_amount, kyt_fee);
Copy link
Member

Choose a reason for hiding this comment

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

Can it be the case that min_retryieve_amount < kyt_fee? I would expect the KYT fee to be already taken into account since there is this PER_REQUEST_KYT_FEE?

Copy link
Contributor

Choose a reason for hiding this comment

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

But is there any previous check before this line?
If there is none, then it makes sense to check here that the amount in the request is not lower than the KYT fee.
It is a bit of a hypothetical situation because the minimum retrieval amount will always be greater than the KYT fee.

if args.amount < min_amount {
Expand Down Expand Up @@ -308,8 +313,13 @@ pub async fn retrieve_btc_with_approval(
}

let _guard = retrieve_btc_guard(caller)?;
let (min_retrieve_amount, btc_network, kyt_fee) =
read_state(|s| (s.retrieve_btc_min_amount, s.btc_network, s.kyt_fee));
let (min_retrieve_amount, btc_network, kyt_fee) = read_state(|s| {
(
s.fee_based_retrieve_btc_min_amount,
s.btc_network,
s.kyt_fee,
)
});
let min_amount = max(min_retrieve_amount, kyt_fee);
if args.amount < min_amount {
return Err(RetrieveBtcWithApprovalError::AmountTooLow(min_amount));
Expand Down
29 changes: 16 additions & 13 deletions rs/bitcoin/ckbtc/minter/tests/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -631,10 +631,11 @@ struct CkBtcSetup {

impl CkBtcSetup {
pub fn new() -> Self {
Self::new_with(Network::Mainnet)
let retrieve_btc_min_amount = 100_000;
Self::new_with(Network::Mainnet, retrieve_btc_min_amount)
}

pub fn new_with(btc_network: Network) -> Self {
pub fn new_with(btc_network: Network, retrieve_btc_min_amount: u64) -> Self {
let bitcoin_id = bitcoin_canister_id(btc_network);
let env = StateMachineBuilder::new()
.with_master_ecdsa_public_key()
Expand Down Expand Up @@ -663,11 +664,6 @@ impl CkBtcSetup {
)
.expect("failed to install the ledger");

let retrieve_btc_min_amount = match btc_network {
Network::Testnet | Network::Regtest => 10_000,
Network::Mainnet => 100_000,
};

env.install_existing_canister(
minter_id,
minter_wasm(),
Expand Down Expand Up @@ -1364,32 +1360,39 @@ fn test_min_retrieval_amount_mainnet() {

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

Choose a reason for hiding this comment

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

Could we also add a test that when the retrieve_btc_min_amount is changed by upgrade, the next fee computation is done with the updated amount?

let ckbtc = CkBtcSetup::new_with(Network::Testnet);
let min_amount = 12_345;
let ckbtc = CkBtcSetup::new_with(Network::Testnet, min_amount);

ckbtc.refresh_fee_percentiles();
let retrieve_btc_min_amount = ckbtc.get_minter_info().retrieve_btc_min_amount;
assert_eq!(retrieve_btc_min_amount, 10_000);
assert_eq!(retrieve_btc_min_amount, min_amount);

// The numbers used in this test have been re-computed using a python script using integers.
ckbtc.set_fee_percentiles(&vec![0; 100]);
ckbtc.refresh_fee_percentiles();
let retrieve_btc_min_amount = ckbtc.get_minter_info().retrieve_btc_min_amount;
assert_eq!(retrieve_btc_min_amount, 10_000);
assert_eq!(retrieve_btc_min_amount, min_amount);

ckbtc.set_fee_percentiles(&vec![116_000; 100]);
ckbtc.refresh_fee_percentiles();
let retrieve_btc_min_amount = ckbtc.get_minter_info().retrieve_btc_min_amount;
assert_eq!(retrieve_btc_min_amount, 60_000);
assert_eq!(retrieve_btc_min_amount, 50_000 + min_amount);
Copy link
Member

Choose a reason for hiding this comment

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

Understanding question: why does the minimal amount of BTC that can be withdrawn vary with the network fees? It's clear to me that the recipient will get less than the withdrawn amount (to pay for the transaction fees) and that if the minimum amount is fixed, then potentially the recipient could get 0 or the transaction not be processed before a long time but AFAIK this should not impact other transactions (contrary to Ethereum)


ckbtc.set_fee_percentiles(&vec![342_000; 100]);
ckbtc.refresh_fee_percentiles();
let retrieve_btc_min_amount = ckbtc.get_minter_info().retrieve_btc_min_amount;
assert_eq!(retrieve_btc_min_amount, 60_000);
assert_eq!(retrieve_btc_min_amount, 50_000 + min_amount);

ckbtc.set_fee_percentiles(&vec![343_000; 100]);
ckbtc.refresh_fee_percentiles();
let retrieve_btc_min_amount = ckbtc.get_minter_info().retrieve_btc_min_amount;
assert_eq!(retrieve_btc_min_amount, 110_000);
assert_eq!(retrieve_btc_min_amount, 100_000 + min_amount);

// When fee becomes 0 again, it goes back to the initial setting
ckbtc.set_fee_percentiles(&vec![0; 100]);
ckbtc.refresh_fee_percentiles();
let retrieve_btc_min_amount = ckbtc.get_minter_info().retrieve_btc_min_amount;
assert_eq!(retrieve_btc_min_amount, min_amount);
}

#[test]
Expand Down