-
Notifications
You must be signed in to change notification settings - Fork 94
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
fix(watchers): align taker fee validation retries with makers #2263
Changes from 4 commits
1bd7b3c
b7781b3
515f6ae
b81bfc6
e05f246
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -1,5 +1,6 @@ | ||||||||
use super::{broadcast_p2p_tx_msg, get_payment_locktime, lp_coinfind, taker_payment_spend_deadline, tx_helper_topic, | ||||||||
H256Json, SwapsContext, WAIT_CONFIRM_INTERVAL_SEC}; | ||||||||
H256Json, SwapsContext, TAKER_FEE_VALIDATION_ATTEMPTS, TAKER_FEE_VALIDATION_RETRY_DELAY_SECS, | ||||||||
WAIT_CONFIRM_INTERVAL_SEC}; | ||||||||
use crate::lp_network::{P2PRequestError, P2PRequestResult}; | ||||||||
|
||||||||
use crate::MmError; | ||||||||
|
@@ -181,24 +182,31 @@ impl State for ValidateTakerFee { | |||||||
|
||||||||
async fn on_changed(self: Box<Self>, watcher_ctx: &mut WatcherStateMachine) -> StateResult<WatcherStateMachine> { | ||||||||
debug!("Watcher validate taker fee"); | ||||||||
let validated_f = watcher_ctx | ||||||||
.taker_coin | ||||||||
.watcher_validate_taker_fee(WatcherValidateTakerFeeInput { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this watcher_validate_taker_fee fn used to fail because inside there is a check that tx.height should be over min_block_number. The case when the tx is in mempool (tx.height is None) is processed and validation does not fail. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But we use confirmations not height komodo-defi-framework/mm2src/coins/utxo/utxo_common.rs Lines 2004 to 2006 in f487947
I think it fails due to the get_block_count delay in having the current height of the blockchain, as the watcher can use a different electrum other than the taker. This is why the retries fix this.
|
||||||||
taker_fee_hash: watcher_ctx.data.taker_fee_hash.clone(), | ||||||||
sender_pubkey: watcher_ctx.verified_pub.clone(), | ||||||||
min_block_number: watcher_ctx.data.taker_coin_start_block, | ||||||||
fee_addr: DEX_FEE_ADDR_RAW_PUBKEY.clone(), | ||||||||
lock_duration: watcher_ctx.data.lock_duration, | ||||||||
}) | ||||||||
.compat(); | ||||||||
|
||||||||
if let Err(err) = validated_f.await { | ||||||||
return Self::change_state(Stopped::from_reason(StopReason::Error( | ||||||||
WatcherError::InvalidTakerFee(format!("{:?}", err)).into(), | ||||||||
))); | ||||||||
}; | ||||||||
|
||||||||
Self::change_state(ValidateTakerPayment {}) | ||||||||
let validation_result = retry_on_err!(async { | ||||||||
watcher_ctx | ||||||||
.taker_coin | ||||||||
.watcher_validate_taker_fee(WatcherValidateTakerFeeInput { | ||||||||
taker_fee_hash: watcher_ctx.data.taker_fee_hash.clone(), | ||||||||
sender_pubkey: watcher_ctx.verified_pub.clone(), | ||||||||
min_block_number: watcher_ctx.data.taker_coin_start_block, | ||||||||
fee_addr: DEX_FEE_ADDR_RAW_PUBKEY.clone(), | ||||||||
lock_duration: watcher_ctx.data.lock_duration, | ||||||||
}) | ||||||||
.compat() | ||||||||
.await | ||||||||
}) | ||||||||
.repeat_every_secs(TAKER_FEE_VALIDATION_RETRY_DELAY_SECS) | ||||||||
.attempts(TAKER_FEE_VALIDATION_ATTEMPTS) | ||||||||
.inspect_err(|e| error!("Error validating taker fee: {}", e)) | ||||||||
.await; | ||||||||
|
||||||||
match validation_result { | ||||||||
Ok(_) => Self::change_state(ValidateTakerPayment {}), | ||||||||
Err(repeat_err) => Self::change_state(Stopped::from_reason(StopReason::Error( | ||||||||
WatcherError::InvalidTakerFee(repeat_err.to_string()).into(), | ||||||||
))), | ||||||||
} | ||||||||
} | ||||||||
} | ||||||||
|
||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about that, Should we allow seed nodes to send failed swap transactions to electrums? My opinion is no, but opening this to discussion. There is also this comment #1238 (comment) to think about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we sending the tx only when running in top of native?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use this in case the electrum/s used by the user in the swap is down or censoring transactions, if any one node in the network have the coin enabled with a native daemon, it will be an additional way to get the transaction through. After your electrums manager PR, we can add all the available electrums to the user's list and since all electrums are tried for broadcasting, this message will be only used as fallback if all electrums fail. Now, one of the seed nodes can have a different electrum which is why I thought about "Should we allow seed nodes to send failed swap transactions to electrums?" but the drawback of this is that this will be resource exhaustive for electrums which is another reason we only send the tx to nodes running the coin on top of native daemons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this mean "send failed swap transactions"? (Are those refund transactions - why then should they be treated differently)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are spend or refund transactions. If electrums censor or fail the broadcasting of these transactions, we rely on nodes running native daemon to get it into the mempool. It should be combined with spv validation in the future, so even if an electrum returns transaction broadcasted successfully, we make sure by validating it and if spv fails we try this p2p transaction helper as a last resort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also wondered what would happen if several nodes in native mode are requested to send same transaction (like banning nodes).
Apparently not, as I can see in kmd daemons code: if a transaction is relayed from some node's mempool to other nodes (which also have the same transaction in mempool) they do not consider the originating node as misbehaving.