Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: add SetPendingTransferProofCourierAddr endpoint to fix erroneous courier addresses #1097

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions perms/perms.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ var (
Entity: "assets",
Action: "read",
}},
"/taprpc.TaprootAssets/SetPendingTransferProofCourierAddr": {{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should go with just UpdateTransferOutput and only allow the proof courier address to be updated for now. And we'll just return an error if the selected transfer output isn't pending.
IMO we don't have to put the whole documentation of a method into its name ;-)

Entity: "assets",
Action: "write",
}},
"/taprpc.TaprootAssets/QueryAddrs": {{
Entity: "addresses",
Action: "read",
Expand Down
4 changes: 2 additions & 2 deletions proof/verifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -504,8 +504,8 @@ type GroupAnchorVerifier func(gen *asset.Genesis,
// *proofVerificationOpts.
type ProofVerificationOption func(p *proofVerificationParams)

// proofVerificationParams is a struct containing various options that may be used
// during proof verification
// proofVerificationParams is a struct containing various options that may be
// used during proof verification
type proofVerificationParams struct {
// ChallengeBytes is an optional field that is used when verifying an
// ownership proof. This field is only populated when the corresponding
Expand Down
114 changes: 114 additions & 0 deletions rpcserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,12 @@ const (
proofTypeReceive = tapdevrpc.ProofTransferType_PROOF_TRANSFER_TYPE_RECEIVE
)

// nolint: lll
type (
// Shorthand gRPC types.
updateCourierAddrReq = taprpc.SetPendingTransferProofCourierAddrRequest
updateCourierAddrResp = taprpc.SetPendingTransferProofCourierAddrResponse

// cacheableTimestamp is a wrapper around a uint32 that can be used as a
// value in an LRU cache.
cacheableTimestamp uint32
Expand Down Expand Up @@ -1357,6 +1362,114 @@ func (r *rpcServer) ListTransfers(ctx context.Context,
return resp, nil
}

// SetPendingTransferProofCourierAddr sets the transfer output proof courier
// address for a pending (undelivered proof) transfer. The anchoring transaction
// of the target transfer must have already been confirmed on-chain.
func (r *rpcServer) SetPendingTransferProofCourierAddr(ctx context.Context,
req *updateCourierAddrReq) (*updateCourierAddrResp, error) {

// Unmarshal the anchor tx hash.
if len(req.AnchorTxid) == 0 {
return nil, fmt.Errorf("anchor txid must be set")
}

anchorTxHash, err := chainhash.NewHashFromStr(req.AnchorTxid)
if err != nil {
return nil, fmt.Errorf("invalid anchor tx hash: %w", err)
}

// Validate the requested new proof courier address.
_, err = proof.ParseCourierAddress(req.NewProofCourierAddr)
if err != nil {
return nil, fmt.Errorf("invalid proof courier address: %w", err)
}

// Unmarshal the transfer output script key public key.
outputScriptKey, err := parseUserKey(req.TransferOutputScriptPubKey)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the pubkey is a safety net to make sure the user really specified the right one?

if err != nil {
return nil, fmt.Errorf("invalid script pub key: %w", err)
}

// Query for pending transfers only.
pendingParcels, err := r.cfg.AssetStore.QueryParcels(
ctx, anchorTxHash, true,
)
if err != nil {
return nil, fmt.Errorf("failed to query parcels: %w", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user will run into this if they specify a completed transfer. Perhaps we should mention this in the error message, so something like "parcel not found in pending parcels"?

}

// Identify the target parcel and modify the proof courier address.
var targetParcel *tapfreighter.OutboundParcel
for idxParcel := range pendingParcels {
pendingParcel := pendingParcels[idxParcel]

// The anchoring transaction of target parcel must have already
// been confirmed on-chain.
if pendingParcel.AnchorTxBlockHash.IsNone() {
continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this matter? Maybe we want to update before we reached a confirmation?
I assume it has something to do with how we insert a new parcel, see comment below.

}

// Investigate every output in the parcel.
for idxOut := range pendingParcel.Outputs {
output := pendingParcel.Outputs[idxOut]

// Skip outputs that don't match the target output.
if !output.ScriptKey.PubKey.IsEqual(outputScriptKey) ||
output.Position != req.TransferOutputPosition {

continue
}

// At this point we've matched on the correct transfer
// output. We will now perform a sanity check to ensure
// check whether a proof should be delivered for this
// output.
shouldDeliverProof, err := output.ShouldDeliverProof()
if err != nil {
return nil, fmt.Errorf("unable to determine "+
"if proof should be delivered for "+
"the given transfer output: %w", err)
}

if !shouldDeliverProof {
return nil, fmt.Errorf("target transfer " +
"output has been identified in a " +
"pending transfer but a proof should " +
"not be delivered for this output")
}

// Modify the proof courier address of the transfer
// output.
pendingParcel.Outputs[idxOut].ProofCourierAddr =
[]byte(req.NewProofCourierAddr)

targetParcel = pendingParcel
break
}

// Break if we've found the target parcel.
if targetParcel != nil {
break
}
}

// Return an error if the target parcel was not found.
if targetParcel == nil {
return nil, fmt.Errorf("target pending transfer output not " +
"found")
}

// Request delivery of the updated parcel.
pendingParcel := tapfreighter.NewPendingParcel(targetParcel)
_, err = r.cfg.ChainPorter.RequestShipment(pendingParcel)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be handled properly and not lead to duplicates? Shouldn't we add the possibility to update an existing parcel instead?

if err != nil {
return nil, fmt.Errorf("error requesting delivery of "+
"modified pending parcel: %w", err)
}

return &updateCourierAddrResp{}, nil
}

// QueryAddrs queries the set of Taproot Asset addresses stored in the database.
func (r *rpcServer) QueryAddrs(ctx context.Context,
req *taprpc.QueryAddrRequest) (*taprpc.QueryAddrResponse, error) {
Expand Down Expand Up @@ -3382,6 +3495,7 @@ func marshalOutboundParcel(
OutputType: rpcOutType,
AssetVersion: assetVersion,
ProofDeliveryStatus: proofDeliveryStatus,
Position: out.Position,
}
}

Expand Down
18 changes: 11 additions & 7 deletions tapfreighter/chain_porter.go
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,7 @@ func (p *ChainPorter) advanceState(pkg *sendPackage, kit *parcelKit) {
// Continue state transitions whilst state complete has not yet
// been reached.
for pkg.SendState < SendStateComplete {
log.Infof("ChainPorter executing state: %v",
pkg.SendState)
log.Infof("ChainPorter executing state: %v", pkg.SendState)

// Before we attempt a state transition, make sure that
// we aren't trying to shut down.
Expand Down Expand Up @@ -763,10 +762,10 @@ func (p *ChainPorter) updateAssetProofFile(ctx context.Context,
}, nil
}

// transferReceiverProof retrieves the sender and receiver proofs from the
// archive and then transfers the receiver's proof to the receiver. Upon
// successful transfer, the asset parcel delivery is marked as complete.
func (p *ChainPorter) transferReceiverProof(pkg *sendPackage) error {
// transferProofs delivers the transfer output proofs to the relevant receiving
// peers. Once successfully delivered, the delivery status of each corresponding
// proof is marked as complete.
func (p *ChainPorter) transferProofs(pkg *sendPackage) error {
ctx, cancel := p.WithCtxQuitNoTimeout()
defer cancel()

Expand Down Expand Up @@ -870,6 +869,9 @@ func (p *ChainPorter) transferReceiverProof(pkg *sendPackage) error {

// If we have a non-interactive proof, then we'll launch several
// goroutines to deliver the proof(s) to the receiver(s).
//
// TODO(ffranr): even if some of these deliveries fail, we should
// continue with the rest of the deliveries.
err := fn.ParSlice(ctx, pkg.OutboundPkg.Outputs, deliver)
if err != nil {
return fmt.Errorf("error delivering proof(s): %w", err)
Expand Down Expand Up @@ -1237,6 +1239,8 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) {
// package state on disk to reflect this. This step frees up the change
// outputs so that they can be used in future transactions.
case SendStateStorePostAnchorTxConf:
// Store the proofs in the proof archive. This step also
// populates the FinalProofs field in the package.
err := p.storeProofs(&currentPkg)
if err != nil {
return nil, fmt.Errorf("unable to store proofs: %w",
Expand Down Expand Up @@ -1266,7 +1270,7 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) {
go func() {
defer p.Wg.Done()

err := p.transferReceiverProof(&currentPkg)
err := p.transferProofs(&currentPkg)
if err != nil {
log.Errorf("unable to transfer receiver "+
"proof: %v", err)
Expand Down
5 changes: 5 additions & 0 deletions taprpc/assetwalletrpc/assetwallet.swagger.json
Original file line number Diff line number Diff line change
Expand Up @@ -1091,6 +1091,11 @@
"proof_delivery_status": {
"$ref": "#/definitions/taprpcProofDeliveryStatus",
"description": "The delivery status of the proof associated with this output."
},
"position": {
"type": "string",
"format": "uint64",
"description": "The position of the output in the transfer output list."
}
}
},
Expand Down
Loading
Loading