[3/?] StaticAddr: Deposit tracking and timeout sweep #677
Conversation
hieblmi
force-pushed
the
static-addr-3
branch
3 times, most recently
from
dde94ea to
b2f400c
Compare
hieblmi
force-pushed
the
static-addr-3
branch
from
b2f400c to
a079dc2
Compare
hieblmi
force-pushed
the
static-addr-3
branch
4 times, most recently
from
75feb5c to
298ba8e
Compare
hieblmi
force-pushed
the
static-addr-3
branch
4 times, most recently
from
2db3756 to
2b1456b
Compare
staticaddr/deposit/fsm.go
Outdated
| // If the deposit is expired but not yet sufficiently confirmed, we | ||
| // republish the expiry sweep transaction. | ||
| if f.deposit.isExpired(currentHeight, params.Expiry) { | ||
| if f.deposit.State == WaitForExpirySweep { |
There was a problem hiding this comment.
Why don't we just send the OnExpiry event here an handle it in the fsm, so that it republishes? This would require WaitForExpirySweepAction to be async, but IMO makes the whole flow more sensible me.
There was a problem hiding this comment.
Hm I guess what speaks against this, is that republishing might error. Which if we'd need to handle that in the fsm itself would be more complicated. So I'm fine with the current approach
There was a problem hiding this comment.
But maybe it's worth to remodel this a bit, as we're currently not saving any sweep txid to the db, which IMO is somewhat important for bookkeeping
There was a problem hiding this comment.
We could in WaitForExpirySweepAction, just call something like buildAndPublishTimeoutSweep(f.ctx, f.cfg) which could return a txid and save it to the db.
This would allow merging the PublishExpiredDeposit and WaitForExpirySweep states. in handleBlockNotification we could just send the onexpiry event and in the fsm call the action itself again when the event is sent.
There was a problem hiding this comment.
we could just send the onexpiry event and in the fsm call the action itself again when the event is sent.
Wouldn't the new merged state action also wait for the confirmation of the sweep and hence block processing of new events until the state action returns a.k.a the expiry got confirmed? So we couldn't call buildAndPublishTimeoutSweep again since we wouldn't receive the event till the sweep is confirmed.
What do you think about extracting the sweep tx once we are in the final state Expired and then update the deposit with it, rather than needing to keep track of replacement txns while the sweep is unconfirmed?
EDIT: In the latest commits I am attaching the confirmed sweep tx id after 3 confirmations.
There was a problem hiding this comment.
As a side note this is also not safe as we're running on the block notification goroutine.
There was a problem hiding this comment.
Thanks yes, I've added a mutex to the deposit and locked all operations on the state.
| $7 | ||
| ); | ||
|
|
||
| -- name: UpdateDeposit :exec |
There was a problem hiding this comment.
It's simpler to use just one UpdateDeposit function but perhaps worth considering a more granular approach where depending on the state change we only update the confirmation height or the expiry txid. WDYT? Obviously this would also mean a slight change in how the FSM works and I could also argue its a bit overly cautious.
There was a problem hiding this comment.
We could do that. Are you thinking that instead of updating in each states entry function we'd update only relevant fields of the deposit at the end of each action through a state-customized update function?
Or we could switch on the state in updateDeposit and call the related update method for the state transition?
There was a problem hiding this comment.
Yes I'm thinking of using a state customized update functions. Also not a blocker for this PR, just an idea for later.
staticaddr/address/log.go
Outdated
| @@ -0,0 +1,29 @@ | |||
| package address | |||
There was a problem hiding this comment.
Another approach to subpackage logging could be to just have one logger in the staticaddr package and then each subpackage just defines a global variable (log) which we assign with a modified UseLogger function.
There was a problem hiding this comment.
That reminds me that I didn't want to clutter the logs too much with subsystem logs. I think we can just summarize all deposit/withdraw/loop-in operations inside the SADDR logger. I think the log becomes easier to read then.
There was a problem hiding this comment.
I placed a single logger into the staticaddr package. For the loop-in I might add a separate logger, but creation/deposits/withdrawals I think can be under the same sublogger.
| } | ||
|
|
||
| depositStates := depoFsm.DepositStatesV0() | ||
| switch params.ProtocolVersion { |
There was a problem hiding this comment.
nit: this check could go just below where we call GetStaticAddressParameters().
There was a problem hiding this comment.
I think it has to stay here since if there'd be a V1 state machine we need the depoFsm struct to retrieve those states.
staticaddr/deposit/fsm.go
Outdated
| // If the deposit is expired but not yet sufficiently confirmed, we | ||
| // republish the expiry sweep transaction. | ||
| if f.deposit.isExpired(currentHeight, params.Expiry) { | ||
| if f.deposit.State == WaitForExpirySweep { |
There was a problem hiding this comment.
As a side note this is also not safe as we're running on the block notification goroutine.
|
Thanks for the review @bhandras, I plan to add some unit tests to increase test coverage. |
We remove the static address client and add its rpcs into the SwapClient
hieblmi
force-pushed
the
static-addr-3
branch
2 times, most recently
from
8ae63de to
3c03fc7
Compare
| $7 | ||
| ); | ||
|
|
||
| -- name: UpdateDeposit :exec |
There was a problem hiding this comment.
Yes I'm thinking of using a state customized update functions. Also not a blocker for this PR, just an idea for later.
staticaddr/address/manager.go
Outdated
| func NewAddressManager(cfg *ManagerConfig) *Manager { | ||
| // NewManager creates a new address manager. | ||
| func NewManager(cfg *ManagerConfig) *Manager { | ||
| log = staticaddr.GetLogger() |
There was a problem hiding this comment.
nit: alternatively, you could do this in the package's init() function too, so it's executed only once at the static initialization stage.
There was a problem hiding this comment.
good idea, obvious in hindsight.
staticaddr/deposit/deposit.go
Outdated
| d.Lock() | ||
| defer d.Unlock() | ||
|
|
||
| d.State = state |
There was a problem hiding this comment.
nit: given we have setter and getter maybe we don't even need to export State.
| args.Error(1) | ||
| } | ||
|
|
||
| func TestManager(t *testing.T) { |
It introduces tracking of deposits to a static address and monitoring of the CSV expiry. Instead of using
RegisterConfirmationsNtfnto detect spends to a pkScript, we poll the lnd account of the imported static address taproot script and notify the main event loop of the manager about new deposits to a static address.Todo:- state machine recovery- listening for timeout tx confirmations to be sure that the client claimed funds.