fix(ckbtc): Fix a problem with retrieve_btc_min_amount setting #2435
Conversation
| @@ -1364,32 +1360,39 @@ fn test_min_retrieval_amount_mainnet() { | |||
|
|
|||
| #[test] | |||
| fn test_min_retrieval_amount_testnet() { | |||
There was a problem hiding this comment.
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?
| 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; |
There was a problem hiding this comment.
understanding question: do you know how those numbers were computed? Are they part of some Bitcoin spec.? (maybe @THLO knows)
There was a problem hiding this comment.
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.
|
|
||
| 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); |
There was a problem hiding this comment.
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)
| @@ -122,6 +122,12 @@ pub fn encode_metrics( | |||
| "Minimum number of ckBTC a user can withdraw.", | |||
| )?; | |||
|
|
|||
| metrics.encode_gauge( | |||
There was a problem hiding this comment.
good idea to have a metric here!
| @@ -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, | |||
There was a problem hiding this comment.
nit: maybe some comment on the Candid file mentioning that this amount varies according to the current network fees may be helpful
| s.btc_network, | ||
| s.kyt_fee, | ||
| ) | ||
| }); | ||
|
|
||
| let min_amount = max(min_retrieve_amount, kyt_fee); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
Here for a lack of a better place: The method estimate_fee_per_vbyte is called as part of 2 tasks:
ProcessLogicRefreshFeePercentiles
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?
There was a problem hiding this comment.
Sounds reasonable to me. Paul, what do you think?
| ((PER_REQUEST_RBF_BOUND | ||
| + PER_REQUEST_VSIZE_BOUND * median_fee_rate |
There was a problem hiding this comment.
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?
The
retrieve_btc_min_amountsetting was both used as a configuration and as a dynamically calculated value based on recent fees. This led to a problem that changing this setting through canister upgrades would be ineffective because it would be overwritten by the calculated value.The fix is to introduce another
fee_based_retrieve_btc_min_amountin the state to hold the calculated value, and letretrieve_btc_min_amountalways retain its initial setting.