-
Notifications
You must be signed in to change notification settings - Fork 317
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(ckbtc): improve error handling in new KYT canister #2263
base: master
Are you sure you want to change the base?
Conversation
rs/bitcoin/kyt/src/fetch.rs
Outdated
state::set_fetched_address(txid, index, address.clone()); | ||
} else { | ||
// This error shouldn't happen unless blockdata is corrupted. | ||
return CheckTransactionRetriable::TransientInternalError(format!( |
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 is this considered a transient error?
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.
After a second thought, I think you are right. This should be an irrecoverable error for 2 reasons:
- The next call to
check_transaction
will return the same error, because the dependency transaction is already fetched into cache, and will not be retried. - The transaction data already passed txid check, so most likely all RPC providers would return the same data, and the error will not go away by trying with another provider.
WDYT?
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.
The transaction data already passed txid check,
To me this seems like the key argument why this error should indeed be irrecoverable.
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.
Good catch @ninegua ! Thanks for the PR!
// inputs of other transactions, so it is okay to treat them as `None`. | ||
outputs.push( | ||
Address::from_script(&output.script_pubkey, bitcoin::Network::from(*btc_network)) | ||
.ok(), |
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.
nit: should we maybe log the errors?
Some valid bitcoin transactions may have output scripts that do not translate to any valid addresses. They are also not the inputs of other transactions.
The correct handling is to ignore such outputs when computing output addresses, and set them to
None
instead. This avoids throwing an error when we shouldn't be.Also add a new test case to ensure correct handling if we do encounter an real error, i.e. when such output becomes the input of a transaction, which certainly would indicate corrupted block data, and in theory should never happen.