feat: add bitcoin transaction fee validation #582
Conversation
EncryptedDkgShares script_pubkey field
the regtest variables
whether a script pub key is related to the signers
Also update a BitcoinTxInfo's subfield and comment
years worth of scriptPubKeys. It now gets at least one scriptPubKey
bitcoin core
transactions and making them (hopefully) easier to understand
| // Normal: we take the deposit transaction as is from the test setup | ||
| // and store it in the database. This is necessary for when we fetch | ||
| // outstanding unfulfilled deposit requests. | ||
| setup.store_deposit_tx(&db).await; |
There was a problem hiding this comment.
At the moment, I do not think that this step is actually necessary since we get the deposit transaction from bitcoin-core. I discovered this along the way and just decided to keep it in, but I don't mind removing it.
There was a problem hiding this comment.
I think this hangs a little together with @matteojug's Q regarding using multiple data-sources for the same (types of) information.
| // First we check that bitcoin-core has a record of the transaction | ||
| // where we think it should be. | ||
| let txid = &self.sweep_txid; | ||
| let Some(sweep_tx) = rpc.get_tx_info(txid, &self.sweep_block_hash).await? else { | ||
| return Err(DepositErrorMsg::SweepTransactionMissing.into_error(req_ctx, self)); | ||
| }; |
There was a problem hiding this comment.
qq: here we rely on bitcoin-core, while usually(?) we seem to take our storage as source of truth; mixing the two could cause some not-so-fun-to-track coherence issues, so why are we not trusting our storage here?
There was a problem hiding this comment.
Yeah, we usually only reach out to the database for this information. Here I used bitcoin-core because I felt that it was much easier than using the database. Specifically, I had thought that I would need to write and test a few more queries before we could get the information that we need. But, I probably miscalculated and maybe we just need one more query.
And I'm not so worried about coherence issues (well, at least not yet). Here the RPC might succeed while the equivalent queries might fail because we haven't processed the bitcoin block yet (but I'm not worried about that either). However, I am worried about bitcoin-core not computing the "UndoData" that is necessary for the RPC to actually work as intended. So when we have time we should fix this up to use the database.
Description
Closes #552.
Changes
as _casts for integers moreScriptPubKeyinEncryptedDkgSharesDbReadtrait for checking whether a script pub key is related to the signersBitcoinTxInfo. Also update aBitcoinTxInfo's subfield and commentTesting Information
This PR updates the integration tests in validation to be a little easier to understand. The downsides to these tests is that the blockchain in the database does not contain forks by default. Nonetheless, the increase in clarity should more than make up for that fact.
Checklist: