chore(ckbtc): add check_transaction_query method to Bitcoin checker

[lpahlavi]

(XC-194) Query interface to retrieve check_transaction status.

The check_transaction call requires cycles to execute due to to the HTTP outcalls it has to perform. However, in the event a transaction and all of its inputs have already been loaded to the cache in heap memory, no HTTP calls are required. This new query method therefore checks whether a transaction status may be obtained using only cached information, and if so, returns it without charging any cycles.

[gregorydemay]

Thanks @lpahlavi for this PR! Some preliminary comments.

[gregorydemay]

I think there I would provide a new input type, e.g. CheckTransactionQueryArgs which would be an enum, to be able to handle the transaction Id given in binary or in String format. This is to avoid having a check_transaction_query_str, which would be the pendant to check_transaction_str

[gregorydemay]

nit: the names of the variants are not too descriptive (Blob/Text) so I would rename them to something like TxIdBin/TxIdStr (or similar) since it should be clear that those are transaction IDs.

[lpahlavi]

Good point! Done.

[gregorydemay]

I'm not sure we need this:

1. CheckTransactionIrrecoverableError::InvalidTransaction would not be seen from the query call which only access the cache, since this error is not recorded in the state
2. CheckTransactionIrrecoverableError::InvalidTransactionId: in that case the query call should just panic.
3. I don't know if need to differentiate CheckTransactionIrrecoverableError::ResponseTooLarge from CheckTransactionQueryStatus::HttpOutcallRequired in the query call.

With all this I would tend to just have an Unknown; variant (like Passed). WDYT @ninegua ?

[lpahlavi]

I think that's a sensible suggestion. I've implemented it as you suggested, @ninegua lmk if you disagree.

[ninegua]

LGTM with the simplified result type for queries.

[lpahlavi]

@gregorydemay Thanks a lot for the feedback! I've implemented your suggested changes, and will wait for @ninegua's +1 before adding all of the required tests.

[lpahlavi]

@ninegua : Do you have any idea why we only return one blocked address here? I tried to keep the same behavior as before, but imo we could also return all blocked addresses, no?

[ninegua]

Yes, it should be totally fine to return all blocked addresses. I don't even remember why it was coded this way to return early

[gregorydemay]

nit: you could have everything under a single match statement

Suggested change

	CheckMode::Normal => {
	if let Some(FetchTxStatus::Fetched(fetched)) = state::get_fetch_status(txid) {
	if let Some(response) = check_no_input_address_is_blocked(&fetched) {
	return response.into();
	}
	}
	CheckTransactionQueryStatus::HttpOutcallRequired.into()
	}
	CheckMode::Normal => match state::get_fetch_status(txid) {
	Some(FetchTxStatus::Fetched(fetched)) => {
	check_for_blocked_input_addresses(&fetched).into()
	}
	Some(
	FetchTxStatus::PendingOutcall
	| FetchTxStatus::PendingRetry { .. }
	| FetchTxStatus::Error(_),
	)
	| None => CheckTransactionQueryResponse::Unknown,
	},

[lpahlavi]

Nice! I didn't realize I could use the | operator inside Some(...). I thought I had to do something like Some(A) | Some(B) | ... | None which didn't look so great... Always happy to learn some new Rust tricks!