Skip to content
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

feat: signers no longer store transactions in the database #1559

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

djordon
Copy link
Contributor

@djordon djordon commented Mar 28, 2025

Description

Closes #1557 and closes #1466.

Changes

  • Drop the transactions.tx column in the signer database.
  • Remove the tx field from the model::Transaction struct.
  • Remove the DbRead::get_bitcoin_tx method, since we cannot implement it with the transactions.tx column removed. Also, it was unused.
  • Remove the BitcoinTx struct.
  • Ignore the in-memory version of the signer UTXO tests. This is because the TestData struct does not correctly populate the database with TxPrevouts or TxOutputs. Might be worth fixing at some later point.

Testing Information

This is kind of a refactor. No new tests, but if the existing tests pass then we should be good. Granted, this PR ignores some tests...

Checklist:

  • I have performed a self-review of my code

@djordon djordon added the sbtc signer binary The sBTC Bootstrap Signer. label Mar 28, 2025
@djordon djordon added this to the sBTC: Recovery and resiliency milestone Mar 28, 2025
@djordon djordon self-assigned this Mar 28, 2025
@djordon djordon changed the title refactor: simplify transaction processing in block observer feat: signers no longer store transactions in the database Mar 28, 2025
}
}

stacks_chain_tip
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq: I'm wondering if returning stacks_chain_tip if no block is found could be an "unexpected" result, vs just panicking. If your test setup was correct you shouldn't (I think?) get here.

@@ -231,7 +231,7 @@ where
/// Assert that the transaction signer will make and store decisions
/// for pending withdraw requests.
pub async fn assert_should_store_decisions_for_pending_withdrawal_requests(self) {
let mut rng = rand::rngs::StdRng::seed_from_u64(46);
let mut rng = rand::rngs::StdRng::seed_from_u64(6);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fails on my machine if I put 13 as seed =(

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it fails in CI env as well: #1566

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, I think it can be ok to make fixing this test out of scope of this PR, remove changes targeted fixing this test and just keep the issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sbtc signer binary The sBTC Bootstrap Signer.
Projects
None yet
3 participants