staticaddr: neutrino fixes #967
Conversation
staticaddr/withdraw/manager.go
Outdated
| @@ -577,7 +580,7 @@ func (m *Manager) publishFinalizedWithdrawalTx(ctx context.Context, | |||
| return false, nil | |||
| } | |||
| } else { | |||
| log.Debugf("published deposit withdrawal with txid: %v", | |||
| log.Debugf("Published deposit withdrawal with txid: %v!", | |||
There was a problem hiding this comment.
nit: when copying the txid from the log the ... and ! might be copied too, not sure if we want that.
There was a problem hiding this comment.
I removed ! and added a space before ....
| @@ -922,10 +922,15 @@ func (d *Daemon) initialize(withMacaroonService bool) error { | |||
| } | |||
| }() | |||
|
|
|||
| // We need a higher timeout here, because withdrawalManager | |||
| // publishes transactions and each PublishTransaction call can | |||
| // wait for getting inv messages from a peer (neutrino). | |||
| return err | ||
| } | ||
| for tx, deposits := range depositsByWithdrawalTx { | ||
| eg.Go(func() error { |
There was a problem hiding this comment.
We should pass in tx and deposits to the goroutine to make sure each goroutine operates on the right variable.
There was a problem hiding this comment.
We can't pass additional arguments to Go. Modern Go has loopvar always enabled, so this should work as expected.
There was a problem hiding this comment.
I think we then should capture txid and deposits as suggested here: https://go.dev/doc/faq#closures_and_goroutines, like
for _, v := range values {
v := v // create a new 'v'.
go func() {
fmt.Println(v)
done <- true
}()
}
There was a problem hiding this comment.
This is no longer needed. See the end of that section:
This behavior of the language, not defining a new variable for each iteration, was considered a mistake in retrospect, and has been addressed in Go 1.22, which does indeed create a new variable for each iteration, eliminating this issue.
|
I pushed another commit |
| @@ -227,15 +227,18 @@ func (m *Manager) recoverWithdrawals(ctx context.Context) error { | |||
| } | |||
|
|
|||
| // Group the deposits by their finalized withdrawal transaction. | |||
| depositsByWithdrawalTx := make(map[*wire.MsgTx][]*deposit.Deposit) | |||
| depositsByWithdrawalTx := make(map[chainhash.Hash][]*deposit.Deposit) | |||
This is needed for neutrino.
Use errgroup to publish each transaction in a separate goroutine. Publishing a transaction can take a while in neutrino mode.
Do not rely on GetActiveDepositsInState reusing the same *wire.MsgTx pointer for a unique transaction.
|
Rebased and pushed another commit with logging fix "staticaddr: fix error formatting in the log". |
Recovered withdrawal transactions are now published in parallel to speed-up starting Loop.
Also increased the timeout given for this startup (including the withdrawals' recovery) from 10s to 1m.
Also added logging and removed an unused method in an interface.
Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixes