stacks-network / sbtc

Repo containing sbtc
GNU General Public License v3.0
254 stars 5 forks source link

[Feature]: Handle lock times in deposit requests #380

Open djordon opened 2 months ago

djordon commented 2 months ago

Feature - Handle lock times in deposit requests

1. Description

The signers need to properly handle the lock times encoded in their deposit scripts in order to properly filter out requests that can be reclaimed "soon".

1.1 Context & Purpose

Right now the signers fetch all deposit requests that have been confirmed for a certain number bitcoin blocks from the chain tip. This is has two problems:

  1. We haven't fixed the relative lock time that depositors must use in their reclaim script. One depositor can lock their reclaim for 150 blocks, while another could for 240 blocks. But even if we fixed the reclaim blocks to, say, 150 blocks (rejecting all deposits that use another value), then we'd still have the following issue.
  2. Relative lock times can be interpreted as either a number of bitcoin blocks or as multiple of 512 seconds measured from the median time past (MTP, the median time of the previous 11 blocks) of the block that confirmed the transaction. The spender of the coins gets to choose, so we need to track the MTP and react on both MTP and the number of confirmed blocks.

2. Technical Details:

We need to track each blocks time and filter deposit requests based both their relative height and the actual time by tracking by the MTP.

2.1 Acceptance Criteria:

3. Related Issues and Pull Requests (optional):

AshtonStephens commented 1 day ago

I'm planning to do this with a few atomic changes:

  1. Add a field to the signer config here that specifies how far from the reclaim script's lock time the deposit needs to be for the signer to accept it. This will be in seconds and we'll interpret blocks to each be 512 seconds
  2. Add a u32 time field to the BitcoinBlock representation in our database that we'll populate here with block.header.time from the Bitcoin library.
  3. When deciding whether to sign for the transaction we'll reject if the reclaim time is too close for comfort here based on the field added in the config earlier.

The too close for comfort will be determined based on the least forgiving interpretation of the closeness of the chain tip. I'm not clear on the best way to assess the median of the last 11 blocks, or rather how best to get the last 11 blocks from the SQL table, but that's how I intend to do it.

@djordon @matteojug any initial thoughts?

djordon commented 1 day ago

1. Add a field to the signer config here that specifies how far from the reclaim script's lock time the deposit needs to be for the signer to accept it.

Yeah a config option is okay but I think using some hard-coded constant is a little better. This isn't something that we want the signer operators to change, at least not yet.

This will be in seconds and we'll interpret blocks to each be 512 seconds

I was thinking that we'd need two values, one for the number of bitcoin blocks and another for actual time.

2. Add a u32 time field to the BitcoinBlock representation in our database that we'll populate here with block.header.time from the Bitcoin library.

Yeah that makes sense.

3. When deciding whether to sign for the transaction we'll reject if the reclaim time is too close for comfort here based on the field added in the config earlier.

Oh, well I don't think that we want to handle this check there. Instead we want to handle it when fetching deposit requests to sweep in, which happens here. As time passes a deposit request will go from being acceptable to unacceptable, so we need to make sure that we catch that.

The too close for comfort will be determined based on the least forgiving interpretation of the closeness of the chain tip.

Yeah that makes the most sense to me.

I'm not clear on the best way to assess the median of the last 11 blocks, or rather how best to get the last 11 blocks from the SQL table, but that's how I intend to do it.

For finding the 11 blocks, you want to do something like the query here, which uses a recursive query to fetch some number of blocks from some starting block hash. That's how we typically identify some part of a blockchain in our queries.

djordon commented 1 day ago

The good folks over at Asymmetric have pointed out something that I had misunderstood (https://github.com/stacks-network/sbtc/issues/516#issuecomment-2438070688). Basically, we the sbtc library should note how a lock time will be interpreted when it does validation. How to do so is specified in BIP-68. This doesn't change the work here that much, just thought that I'd point this out.