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: btc ingress egress tracking #4133

Merged
merged 7 commits into from
Oct 19, 2023
Merged

Conversation

msgmaxim
Copy link
Contributor

Pull Request

Closes: PRO-868 and PRO-916

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

This compliments existing "mempool" tracking by allowing an rpc client to subscribe to BTC transactions. This PR is best to be reviewed commit-by-commit.

  • similar to DOT witnessing, I extracted BTC egress procedure so it can be reused.
  • renamed existing btc module to btc_mempool
  • added btc tracking (which closely remesbles engine's witnessing)
  • BTC mempool tracking now uses DepositTrackerSettings

Note that I also changed BTC client poll internals from 10s to 5s. This is to account for localnet where block time is ~6s or so and would result in missing blocks (mostly doesn't matter, but I want to avoid future surprises when people try this on localnet). I think this is a reasonable change for engine witnessing on mainnet too (I would decrese it even further personally).

I know that continuous adapter is meant to fill in the gaps (and I even experimented with it) but it has some side effects like prosessing blocks from the start of the epoch (which I imagine we don't want for the tracker) and expecting a database. Long term I wouldn't mind an adapter of some sort which would just ensure we don't skip blocks (in some rare edge cases) and not worry about epochs/databases, but for now it is not srictly necessary since we already don't guarantee 100% reliability in the tracker.

Also, I discovered that the current engine's witnesser delays blocks by ~20 seconds even when safety margin isn't used, so the current tracker doesn't process blocks as early as we would hope (I created a separate issue to address this: https://linear.app/chainflip/issue/PRO-923/remove-block-delay-in-ingress-egress-tracker)

@msgmaxim msgmaxim requested a review from kylezs October 18, 2023 06:09
@linear
Copy link

linear bot commented Oct 18, 2023

PRO-868 Ingress Egress Tracker

This will/should be broken down into smaller tickets after we've discussed it.

Problem:

The swapping app currently has to wait for the CFE witnessing and respective confirmation on the SC, both for ingress, when the user is depositing funds, and for egress, when the user is receiving funds. This duration can be quite long, because of the added safety margins to protect against reorgs.

Solution:

While the reorg safety is important for the protocol it's not important for the product. We can use the same code that we use in the CFE witnessing to avoid duplicating the work, but instead redirect the results of that.

Requirements:

  • The Calls that we would normally send to the SC as an extrinsic, can instead be sent down a WS stream for consumption by product team (it doesn't have to be WS according to product team, but I think WS will be simplest for us to do)
  • Reusing the same witnessing components as the engine currently does where possible.
  • Single binary for the trackers of all the chains - this includes the BTC deposit tracker, so would unify / use that deposit tracker binary alongside the code for this
  • Configurable, so it can be used for different networks
    • SC WS endpoint
    • A single, (no need for backup) WS and HTTP endpoints for each chain

Useful info:

  • feat: simple pre-witnessing #4056 / PRO-852
    • Example of how the egress components would be pulled out, as well as the usage of the ProcessCall, and how it can be used to pass in closures to send different data

Out of scope:

  • State, there's no need to hold state in the tracker, either in memory, or in a database. We can be optimistic. If the tracker goes down that's ok, the UI can fallback on the SC BroadcastSuccess event which it already uses, it'll just be a bit slower from the user's perspective for now, but for now that's ok.

If you have questions for the product team and what they need, you can reach out to nat

PRO-916 BTC ingress-egress tracker settings to be unified into DepositTrackerSettings

The settings for BTC in the ingress-egress tracker are read from environment on every call. They should instead be read in at the same time as the other settings.

@codecov
Copy link

codecov bot commented Oct 18, 2023

Codecov Report

Merging #4133 (56c7116) into main (03c4f3c) will increase coverage by 0%.
Report is 12 commits behind head on main.
The diff coverage is 0%.

@@          Coverage Diff           @@
##            main   #4133    +/-   ##
======================================
  Coverage     72%     72%            
======================================
  Files        378     378            
  Lines      60663   60898   +235     
  Branches   60663   60898   +235     
======================================
+ Hits       43495   43675   +180     
- Misses     14904   14956    +52     
- Partials    2264    2267     +3     
Files Coverage Δ
engine/src/witness/btc/btc_source.rs 0% <ø> (ø)
engine/src/witness/btc.rs 27% <0%> (-2%) ⬇️

... and 14 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kylezs
Copy link
Contributor

kylezs commented Oct 18, 2023

I would decrese it even further personally

Fwiw - it's not completely free to do this. the cost of compute on the engine isn't the limiting factor, but how much we're potentially consuming in request limits on any paid PRC provider operators might be using.

(I think 5 seconds is fine for now though, but probably wouldn't go any faster).

.script_pubkey(),
// TODO: Ideally we can submit an empty type here. For
// Bitcoin and some other chains fee tracking is not
// necessary. PRO-370.
Copy link
Contributor

Choose a reason for hiding this comment

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

while we're here, PRO-370 is cancelled, we can delete this comment. we will want to track the fee for btc, but that'll be part of another issue


let btc_source = BtcSource::new(btc_client.clone()).shared(scope);

let strictly_monotonic_source = btc_source.strictly_monotonic().then({
Copy link
Contributor

Choose a reason for hiding this comment

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

We can inline this, no need to do the vaults query in between.

let vaults = epoch_source.vaults().await;

strictly_monotonic_source
.clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary clone - we need to use the shared adapter here instead - and it's not required on the source

As in: https://linear.app/chainflip/issue/PRO-925/use-shared-adapter-inside-the-chunking-adapters

@msgmaxim msgmaxim merged commit 2768d3c into main Oct 19, 2023
42 checks passed
@msgmaxim msgmaxim deleted the feat/btc-ingress-egress-tracking branch October 19, 2023 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants