multi: add new proof.Courier interface for async proof distribution #132
Conversation
|
The open design question is: how do we conclude the ACK sequence between sender / receiver? Given that it's a Two Generals' Problem |
Roasbeef
force-pushed
the
proof-courier
branch
from
5aefabe
to
5f69d3b
Compare
| @@ -247,7 +247,7 @@ func (p *ChainPorter) taroPorter() { | |||
| func (p *ChainPorter) waitForPkgConfirmation(pkg *OutboundParcelDelta) { | |||
| defer p.Wg.Done() | |||
|
|
|||
| ctx, cancel := p.WithCtxQuit() | |||
| ctx, cancel := p.WithCtxQuitNoTimeout() | |||
There was a problem hiding this comment.
Similar case here, this context is just for the CurrentHeight call which shouldn't take too long.
| @@ -269,7 +269,7 @@ func (p *ChainPorter) waitForPkgConfirmation(pkg *OutboundParcelDelta) { | |||
| return | |||
| } | |||
|
|
|||
| confCtx, confCancel := p.WithCtxQuit() | |||
| confCtx, confCancel := p.WithCtxQuitNoTimeout() | |||
| @@ -515,7 +515,7 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { | |||
| // we'll perform coin selection to see if the send request is even | |||
| // possible at all. | |||
| case SendStateCommitmentSelect: | |||
| ctx, cancel := p.WithCtxQuit() | |||
| ctx, cancel := p.WithCtxQuitNoTimeout() | |||
There was a problem hiding this comment.
This and all following changes are for contexts that are for synchronous lnd RPC calls that IMO we should guard with a timeout. But perhaps we want to guard against being cancelled by a shutdown request?
| // On restart, we'll get an error that the output has already | ||
| // been added to the wallet, so we'll catch this now and move | ||
| // along if so. | ||
| case strings.Contains(err.Error(), "already exists"): |
There was a problem hiding this comment.
Oops, I think this is wrong in the custodian as well.
Could you add a fix for this too, please? https://github.com/lightninglabs/taro/blob/main/tarogarden/custodian.go#L400
| defer cancel() | ||
|
|
||
| assetID := addr.ID() | ||
| proof, err := c.cfg.ProofCourier.ReceiveProof( |
There was a problem hiding this comment.
I wonder if we instead should task the proof archive with this? Something along the lines of "hey archive, I'm expecting a proof for this address, can you deliver it to me when available"? Then we don't even need to call the import manually.
| @@ -58,6 +58,10 @@ type ChainPorterConfig struct { | |||
| // TODO(roasbeef): replace with proof.Courier in the future/ | |||
| AssetProofs proof.Archiver | |||
|
|
|||
| // ProofCourier is used to optionally deliver the final proof to the | |||
| // user using an asynchronous transport mechanism. | |||
| ProofCourier proof.Courier[address.Taro] | |||
There was a problem hiding this comment.
Maybe this should instead be added to the proof archive (see comment above).
| return nil, err | ||
| } | ||
|
|
||
| // TODO(roasbeef): modify ACK based on size of ting? |
There was a problem hiding this comment.
I wonder if we could instead use the go-back-N implementation we build for LNC for this as well? Maybe overkill for now as this is temporary as well...
| // Once we receive this ACK, we can clean up our mailbox and also the | ||
| // receiver's mailbox. | ||
| if err := h.mailbox.CleanUp(ctx, senderStreamID); err != nil { | ||
| return err |
There was a problem hiding this comment.
Catch and ignore "stream not found" error and make sure we still attempt to clean up the receiver mailbox too, even if the sender one errors out?
| scriptKey := addr.ScriptKey.SerializeCompressed() | ||
| scriptKey[32] ^= 0x01 | ||
|
|
||
| sid := sha512.Sum512(scriptKey) |
There was a problem hiding this comment.
We should flip the last bit of the SHA512 hash to not break some of the assumptions made in the metrics for the mailbox.
Hmm, and thinking about those metrics, maybe we should use the Pool mailbox instance instead to not mess with the statistics of TW?
The WithCtxQuit has an implicit timeout of 30 seconds. We were using that to pass into the conf registration call, which ofc can take longer than 30 seconds on an actual chain.
Tracking the issue on the ind side with: lightningnetwork/lnd#6952
The transfer tx isn't populated yet, so this would panic.
In this commit, we add a new `proof.Courier` interface that's optionally used as async proof distribution. The current default implementation uses a hashmail server to send the proofs back and forth between the sender and the receiver. In the future, this'll be replaced with streaming/long-polling connections to a desginated multi-verse. Fixes #95
Roasbeef
force-pushed
the
proof-courier
branch
from
5f69d3b
to
7cc14e7
Compare
…of clean up This ensures we don't wait too long if we want to immediately shutdown, and also ensures that even if we're shutting down, we'll try to complete the last phase of the send process. Resolves some review comments from #132.
In this commit, we add a new
proof.Courierinterface that's optionally used as async proof distribution. The current default implementation uses a hashmail server to send the proofs back and forth between the sender and the receiver. In the future, this'll be replaced with streaming/long-polling connections to a desginated multi-verse.Fixes #95