staticaddr: allow rbf'ing withdrawal transactions #892
Conversation
hieblmi
force-pushed
the
resign-withdrawal
branch
2 times, most recently
from
f868520 to
4031671
Compare
hieblmi
force-pushed
the
resign-withdrawal
branch
2 times, most recently
from
49e8c1d to
6b1f478
Compare
hieblmi
force-pushed
the
resign-withdrawal
branch
13 times, most recently
from
8eb6597 to
5151ca2
Compare
hieblmi
force-pushed
the
resign-withdrawal
branch
4 times, most recently
from
8a74aae to
91ea1cf
Compare
bhandras
left a comment
bhandras
left a comment
There was a problem hiding this comment.
LGTM, thanks for the iterations on the design 🎉
staticaddr/withdraw/manager.go
Outdated
| // We also avoid that the user selects a subset of previously | ||
| // clustered deposits for a fee bump. This would result in a | ||
| // different transaction shape. | ||
| allDeposits, err := m.cfg.DepositManager.GetActiveDepositsInState( |
There was a problem hiding this comment.
We can avoid this call. Previously we got a transaction in deposits[i].FinalizedWithdrawalTx.
Can't we verify that the transaction has the same set of deposits as deposits?
If we make sure that all deposits are unique, we can just compare len(deposits) == len(prevTx.TxIn).
staticaddr/withdraw/manager.go
Outdated
| // the withdrawal transaction. | ||
| // Since we ensure above that the same ensemble of deposits is | ||
| // republished in case of a fee bump, it suffices if only one spent | ||
| // notifier is run, since if the first input is spent. |
There was a problem hiding this comment.
What happens if loopd restarts after first withdrawal tx is published? Will it listen for spending after the restart?
There was a problem hiding this comment.
yes, after a restart recoverWithdrawals will republish the latest version of the withdrawal tx and monitor the spending.
staticaddr/withdraw/manager.go
Outdated
| for _, d := range deposits { | ||
| err = m.cfg.DepositManager.UpdateDeposit(ctx, d) | ||
| if err != nil { | ||
| return "", "", err |
There was a problem hiding this comment.
I propose to use fmt.Errorf to attach an error message to simplify debugging.
|
|
||
| confChan, errChan, err := m.cfg.ChainNotifier.RegisterConfirmationsNtfn( | ||
| ctx, &txHash, withdrawalPkScript, MinConfs, | ||
| int32(m.initiationHeight.Load()), |
There was a problem hiding this comment.
MinConfs is now not used. What if a reorg happens? Shouldn't we run RegisterConfirmationsNtfn after a successful spending (1 conf) before transitioning the deposit to Withdrawn state?
There was a problem hiding this comment.
nice catch, I re-added the confirmation listener.
| !strings.Contains(err.Error(), "insufficient fee") { | ||
| if !strings.Contains(err.Error(), chain.ErrSameNonWitnessData.Error()) && | ||
| !strings.Contains(err.Error(), "output already spent") && | ||
| !strings.Contains(err.Error(), chain.ErrInsufficientFee.Error()) { |
There was a problem hiding this comment.
I propose to factor out this check to a function and reuse everywhere (e.g. in sweepbatcher package).
Also this commit should be squashed into commit staticaddr: allow rbf'ing withdrawal transactions.
hieblmi
force-pushed
the
resign-withdrawal
branch
from
91ea1cf to
94fa46f
Compare
hieblmi
force-pushed
the
resign-withdrawal
branch
4 times, most recently
from
1db759e to
0a0806d
Compare
|
@sputn1ck: review reminder |
hieblmi
force-pushed
the
resign-withdrawal
branch
3 times, most recently
from
b1ee0d2 to
80fff99
Compare
hieblmi
force-pushed
the
resign-withdrawal
branch
from
80fff99 to
b5af507
Compare
4 participants
This PR adds the ability to request signatures for higher fee versions of previously published withdrawal transactions.