Fix sidecar message handlers #341
Conversation
positiveblue
force-pushed
the
fix-sidecar-message-handlers
branch
from
d0c07e0 to
8f04e75
Compare
There was a problem hiding this comment.
Great fixes and nice find! A couple of comments about error wrapping. But I'm pretty confident you found the underlying cause for the problem Voltage described.
I think we should add a server side integration test for this specific case, pulling in the version of this PR to verify.
sidecar_acceptor.go
Outdated
| @@ -617,8 +595,7 @@ func (a *SidecarAcceptor) matchPrepare(pendingBatch *order.Batch, | |||
| // that's being used to pay for the sidecar channel. | |||
| err = a.cfg.FundingManager.PrepChannelFunding(batch, a.getSidecarAsOrder) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("error preparing channel funding: %v", | |||
| err) | |||
| return fmt.Errorf("error preparing channel funding: %v", err) | |||
There was a problem hiding this comment.
We need to wrap this error, otherwise the errors.As(failure, &partialReject): below won't trigger.
positiveblue
force-pushed
the
fix-sidecar-message-handlers
branch
3 times, most recently
from
a51e447 to
db55432
Compare
|
@positiveblue, remember to re-request review from reviewers when ready |
| } | ||
|
|
||
| // We know we're involved in a batch, so let's store it for the | ||
| // next step. | ||
| a.pendingBatch = newBatch |
There was a problem hiding this comment.
Why do we no longer store the batch here? We'll need to remove it if the execution fails for w/e reason.
There was a problem hiding this comment.
Yes, we do, but all the logic is inside the a.matchPrepare, a.matchSign and a.matchFinalize
| return nil, fmt.Errorf("unable to cleanup previous "+ | ||
| "batch: %v", err) | ||
| if a.pendingBatch != nil { | ||
| if err := a.removeShims(a.pendingBatch); err != nil { |
There was a problem hiding this comment.
So this is part of the core fix here? That we wipe the batch after we remove the shim?
There was a problem hiding this comment.
Yes, the fix was merged in another PR (already in master). Usually we want to remove the shims of the previous batch, but it was possible to end up in a state were there was not previous batch stored and some shims were created
| return batch, nil | ||
| // We know we're involved in a batch, so let's store it for the | ||
| // next step. | ||
| a.pendingBatch = batch |
There was a problem hiding this comment.
Ok I see we only store it now once all the other operations succeed...
|
|
||
| // Assert we're in the correct state to receive a sign message. | ||
| if !a.isPending(msg.BatchId) { | ||
| return fmt.Errorf("pending batchID was: %x got: %x", |
There was a problem hiding this comment.
Seems this would indicate a logic error elsewhere if we see it frequently in practice, or even a bug in the server's execution logic.
There was a problem hiding this comment.
Agreed, it should not be the normal flow. I have never seen it in our logs, but the code was already there and I think it does not hurt us to keep it. (I hope we end up in this state though*)
positiveblue
force-pushed
the
fix-sidecar-message-handlers
branch
from
db55432 to
c06ee37
Compare
positiveblue
force-pushed
the
fix-sidecar-message-handlers
branch
from
c06ee37 to
cb86b2c
Compare
|
Running this branch on my machine. I am hopeful that this fixes it! |
|
But I am also hopeful that your backend fixes also contribute to better reliability even without upgrading devices to the PR |
rpc: parse unknown order type as peer dependent
The first commit cleans up the shims whenever we reject a batch.
The second commit populates the
OrderMatchRejectmsg properly.