sweepbatcher: fix some minor bugs #871
Conversation
starius
changed the title
| @@ -1872,7 +1876,10 @@ func (b *batch) monitorConfirmations(ctx context.Context) error { | |||
| func getFeePortionForSweep(spendTx *wire.MsgTx, numSweeps int, | |||
| totalSweptAmt btcutil.Amount) (btcutil.Amount, btcutil.Amount) { | |||
|
|
|||
| totalFee := int64(totalSweptAmt) - spendTx.TxOut[0].Value | |||
| totalFee := int64(totalSweptAmt) | |||
| if len(spendTx.TxOut) > 0 { | |||
There was a problem hiding this comment.
if there is no TxOut then totalSweptAmt would be considered as totalFee which doesn't seem right. Should we handle an error in an else clause?
There was a problem hiding this comment.
If there are no outputs, it means the entire amount was used for miner fees, so the calculation is correct.
That said, this scenario is undesirable, and the user should be warned if it occurs. The function is invoked from handleSpend, which already logs a warning in such cases.
For this situation to arise, the sweeping side would need to be offline (not sweeping) for the entire period, and the other side would have to intentionally choose to burn the coin instead of collecting it. While this is highly unlikely, it is technically possible.
There was a problem hiding this comment.
thanks for the clarification.
sweepbatcher/sweep_batch.go
Outdated
| // If initialDelay is 0, it does not fire to prevent race with | ||
| // blockChan which also fires immediately with current tip. Such a race | ||
| // may result in double publishing if batchPublishDelay is also 0. | ||
| // If initialDelay is 0, it does not fire not to install timerChan twice |
There was a problem hiding this comment.
nit: "it does not fire to not install timerChan"
There was a problem hiding this comment.
Rephrased the whole sentence:
// initialDelayChan is a timer which fires upon initial delay end.
// If initialDelay is set to 0, it will not trigger to avoid setting up
// timerChan twice, which could lead to double publishing if
// batchPublishDelay is also 0.
starius
force-pushed
the
sweepbatcher-fixes
branch
from
e1ceb9d to
a7bd90a
Compare
Previously the code handling spending tx crashed if it doesn't have an output. This is likely to occur only in tests.
Prevent a crash with "a height hint greater than 0 must be provided" error when monitorSpend starts at the beginning of batch.Run. The timer timerChan is now initialized at the start, because it was previously initialized after the first block (the current tip) was read from blockChan and now the first block is read before the main for-select loop to fill the field currentHeight in advance.
starius
force-pushed
the
sweepbatcher-fixes
branch
from
a7bd90a to
6efd010
Compare
|
Fixed a typo in the commit message. |
Fix minor bugs identified during testing.
batch.currentHeightbeforehand, ensuring it's set beforemonitorSpendis launched. Without this, the entire batcher could crash with the error: "a height hint greater than 0 must be provided."panic.batch.blockEpochChanfield.Pull Request Checklist
release_notes.mdif your PR contains major features, breaking changes or bugfixes