legacyrpc: add fundrawtransaction command #1195

vctt94 · 2018-06-27T14:24:23Z

jrick · 2018-07-12T18:30:29Z

rpc/legacyrpc/methods.go

@@ -31,6 +31,8 @@ import (
 	"github.com/decred/dcrwallet/wallet"
 	"github.com/decred/dcrwallet/wallet/txrules"
 	"github.com/decred/dcrwallet/wallet/udb"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"


there should be no grpc imports in the json-rpc implementation (and likewise, no json-rpc code in the grpc server)

vctt94 · 2018-08-06T17:24:49Z

This PR is ready for another review

jrick · 2018-10-05T13:36:43Z

rpc/legacyrpc/methods.go

+	if err != nil {
+		return nil, rpcError(dcrjson.ErrRPCDeserialization, err)
+	}
+	err = mtx.Deserialize(bytes.NewReader(decodedTx))


This and the hex decoding may be done in a single step by using mtx.Deserialize(hex.NewDecoder(strings.NewReader(cmd.HexString)))

jrick · 2018-10-05T13:38:28Z

rpc/legacyrpc/methods.go

+	}
+	resp := &dcrjson.FundRawTransactionResult{
+		Hex: hex.EncodeToString(buf.Bytes()),
+		Fee: float64(fundedTx.EstimatedSignedSerializeSize) / 1e8,


This is not the correct fee calculation. The fee is totalInputValue - totalOutputValue

jrick · 2018-10-05T13:39:58Z

rpc/legacyrpc/methods.go

+	}
+	feePerKb := w.RelayFee()
+	requiredConfs := int32(1)
+	addr := ""


This should be of type dcrutil.Address. Address decoding should occur in this RPC handler and not in the wallet method.

wallet/addresses.go

jrick · 2018-10-05T13:46:24Z

wallet/createtx.go

@@ -168,6 +168,70 @@ func (w *Wallet) NewUnsignedTransaction(outputs []*wire.TxOut, relayFeePerKb dcr
 	return authoredTx, nil
 }

+func (w *Wallet) FundRawTransaction(inputs []*wire.TxIn, outputs []*wire.TxOut, relayFeePerKb dcrutil.Amount, account uint32, minConf int32,


There are other fields besides the inputs and outputs that must be preserved by fundrawtransaction (e.g. sertype, version, locktime, and expiry), so we can not just pass in the inputs and outputs of the decoded input and then return an entirely new transaction.

Use a dcrutil.Address for the change address.

jrick · 2018-10-05T13:53:34Z

wallet/txauthor/author.go

@@ -154,6 +154,82 @@ func NewUnsignedTransaction(outputs []*wire.TxOut, relayFeePerKb dcrutil.Amount,
 	}
 }

+func FundRawTransaction(inputs []*wire.TxIn, outputs []*wire.TxOut, relayFeePerKb dcrutil.Amount,


Again, we must pass a MsgTx directly for all tx fields, not just inputs and outputs.

wallet/txauthor/author.go

jrick · 2018-10-05T13:57:31Z

wallet/txauthor/author.go

+
+	totalInTransaction := h.SumInputValues(inputs)
+	targetAmount := h.SumOutputValues(outputs) - totalInTransaction
+	scriptSizes := []int{txsizes.RedeemP2PKHSigScriptSize}


This looks wrong. The initial scriptSizes slice should contain all of the sizes of transaction input scripts that were passed to the function. We can not remove any of these inputs.

I changed the initial scriptSizes to this plus each input the transaction might be, is that right now?

scriptSizes := []int{txsizes.RedeemP2PKHSigScriptSize} for range mtx.TxIn { scriptSizes = append(scriptSizes, txsizes.RedeemP2PKHInputSize) }

No, if the inputs already contain scripts, then we should use those sizes. If they are missing, we're stuck since we don't really know what kinds of inputs are being redeemed and how large the input scripts will be once signed. We could guess it to be P2PKH, but that's still a guess and could result in fees being either too high or too low once the transaction is completely signed.

jrick · 2018-10-05T13:59:11Z

wallet/txauthor/author.go

+			maxSignedSize = txsizes.EstimateSerializeSize(scriptSizes,
+				unsignedTransaction.TxOut, 0)
+		}
+		return &AuthoredTx{


AuthoredTx is the wrong type to return from this function. This is because PrevScripts is not possible to determine if the funded transaction already contained inputs that the wallet did not know about.

jrick · 2018-10-05T13:59:25Z

wallet/txauthor/author.go

+			return nil, errors.E(op, errors.InsufficientBalance)
+		}
+
+		scriptSizes = []int{}


vctt94 · 2018-10-16T15:36:19Z

Thanks for reviewing my PR again Jrick.

I fixed your comments and this PR is ready for another review.

jrick · 2018-10-17T13:56:58Z

internal/rpchelp/helpdescs_en_US.go

+	"fundrawtransaction-options":     "The optional inputs to fund a raw transaction",
+
+	// FundRawTransactionOptions
+	"fundrawtransactionoptions-changeaddress": "Address the change will be send to (default=\"default\")",


The default is the empty string

jrick · 2018-10-17T13:59:08Z

rpc/legacyrpc/methods.go

+	var changeAddr dcrutil.Address
+
+	if cmd.Options != nil {
+		// use provided fee per Kb if specified


it's kilobyte, kB. You will sometimes see KB used in variable names due to camel case

jrick · 2018-10-17T13:59:57Z

rpc/legacyrpc/methods.go

+			requiredConfs = *cmd.Options.ConfTarget
+		}
+		// use provided change account if specified
+		// use default account otherwise


change address, not account

jrick · 2018-10-17T14:06:45Z

wallet/addresses.go

+type p2PKHAddrChangeSource struct {
+	persist       persistReturnedChildFunc
+	changeAddress dcrutil.Address
+	wallet        *Wallet


since this represents a user-specified address, the persist and wallet fields aren't needed (there's no need to persist this address to the wallet's db).

jrick · 2018-10-17T14:12:12Z

wallet/createtx.go

@@ -160,6 +160,62 @@ func (w *Wallet) NewUnsignedTransaction(outputs []*wire.TxOut, relayFeePerKb dcr
 	return authoredTx, nil
 }

+func (w *Wallet) FundRawTransaction(tx wire.MsgTx, relayFeePerKb dcrutil.Amount, account uint32, minConf int32,
+	changeAddress dcrutil.Address) (*wire.MsgTx, error) {


this is an exported public API and needs a documentation comment describing what it does. Be sure to document that changeAddress may be nil, and how the behavior changes depending on if it is nil or not.

tx should be passed by pointer.

jrick · 2018-10-17T14:13:54Z

wallet/createtx.go

+				changeAddress: changeAddress,
+				wallet:        w,
+			}
+			fundedTx, err = txauthor.FundRawTransaction(tx, relayFeePerKb,


this call is identical in both branches of the if-else, so it can be moved out. Declare a change source interface variable above the branch and use the branches to set it to either the provided address or a wallet-derived one.

jrick · 2018-10-17T14:14:24Z

wallet/txauthor/author.go

@@ -154,6 +154,75 @@ func NewUnsignedTransaction(outputs []*wire.TxOut, relayFeePerKb dcrutil.Amount,
 	}
 }

+func FundRawTransaction(mtx wire.MsgTx, relayFeePerKb dcrutil.Amount,


needs doc comment

mtx should be passed by pointer.

jrick · 2018-10-17T14:16:48Z

wallet/txauthor/author.go

+
+	inputs := mtx.TxIn
+	outputs := mtx.TxOut
+	totalInTransaction := h.SumInputValues(inputs)


have to be careful here. If the input amount is not set in any of the inputs (using the sentinel value wire.NullValueIn) this won't be accurate. This has to be checked, and we have to error out if it's not set (it will eventually become required by consensus).

Not sure what this wire.NullValuIn is supposed to do. I added a verification checking if the TxIn array is not empty, is that right? if len(inputs) > 0 {

jrick · 2018-10-17T14:19:52Z

wallet/txauthor/author.go

+
+	totalInTransaction := h.SumInputValues(inputs)
+	targetAmount := h.SumOutputValues(outputs) - totalInTransaction
+	scriptSizes := []int{txsizes.RedeemP2PKHSigScriptSize}


No, if the inputs already contain scripts, then we should use those sizes. If they are missing, we're stuck since we don't really know what kinds of inputs are being redeemed and how large the input scripts will be once signed. We could guess it to be P2PKH, but that's still a guess and could result in fees being either too high or too low once the transaction is completely signed.

jrick · 2018-10-17T14:23:58Z

wallet/txauthor/author.go

+			return nil, errors.E(op, errors.InsufficientBalance)
+		}
+
+		scriptSizes = append(scriptSizes, inputDetail.RedeemScriptSizes...)


this appends the discovered inputs to the script sizes, but the code below may continue the loop without actually adding any of the chosen inputs to the transaction

vctt94 · 2018-10-30T21:53:33Z

I added the fee estimation as we discussed and this PR is ready for another review.

vctt94 · 2019-02-26T15:16:04Z

rebased and moved fundrawtransaction to jsonrpc/types modules

jrick · 2020-03-25T21:07:57Z

Implemented in #1706

vctt94 changed the title ~~add fundrawtransaction command~~ legacyrpc: add fundrawtransaction command Jun 27, 2018

vctt94 mentioned this pull request Jun 27, 2018

dcrjson: add fundrawtransaction command decred/dcrd#1316

Merged

vctt94 force-pushed the addFundrawtransaction branch 2 times, most recently from bd95579 to 7401a62 Compare July 12, 2018 11:41

vctt94 changed the title ~~legacyrpc: add fundrawtransaction command~~ [wip] legacyrpc: add fundrawtransaction command Jul 12, 2018

vctt94 changed the title ~~[wip] legacyrpc: add fundrawtransaction command~~ legacyrpc: add fundrawtransaction command Jul 12, 2018

vctt94 force-pushed the addFundrawtransaction branch from 6855f06 to 84751bc Compare July 12, 2018 14:33

jrick requested changes Jul 12, 2018

View reviewed changes

vctt94 force-pushed the addFundrawtransaction branch 2 times, most recently from 66220b2 to 2d5208e Compare August 1, 2018 15:50

vctt94 force-pushed the addFundrawtransaction branch 2 times, most recently from e34ff63 to 5f82c26 Compare September 13, 2018 13:31

jrick requested changes Oct 5, 2018

View reviewed changes

vctt94 force-pushed the addFundrawtransaction branch 2 times, most recently from 01b1872 to fbe0f48 Compare October 16, 2018 15:32

jrick requested changes Oct 17, 2018

View reviewed changes

vctt94 force-pushed the addFundrawtransaction branch from e64357d to 2f5d70a Compare October 24, 2018 17:17

vctt94 added 10 commits February 26, 2019 10:56

add fundrawtransaction command

87a5181

Fix method to use optional parameters json

61e9da3

fix grpc imports

6f64fd4

fix reviews

cd34c98

dcrd sync

5f7d223

change fundrawtransaction to use txAuthor

0c31467

update method names

a09d4f9

Fix review comments

e5bbe2c

Review fixes. Still left fee estimation

608bc41

Add fee estimation

be0777b

Remove targetAmount from input verification

05ffb81

vctt94 force-pushed the addFundrawtransaction branch 2 times, most recently from 8eb4f7b to bad05ae Compare February 26, 2019 14:29

move fundrawtransaction to types module

b9d5954

vctt94 force-pushed the addFundrawtransaction branch from bad05ae to b9d5954 Compare February 26, 2019 14:46

make travis happy

419e587

jrick mentioned this pull request Mar 25, 2020

Implement JSON-RPC fundrawtransaction #1706

Merged

jrick closed this Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

legacyrpc: add fundrawtransaction command #1195

legacyrpc: add fundrawtransaction command #1195

vctt94 commented Jun 27, 2018

jrick Jul 12, 2018

vctt94 Jul 14, 2018

vctt94 commented Aug 6, 2018

jrick Oct 5, 2018

jrick Oct 5, 2018

jrick Oct 5, 2018

jrick Oct 5, 2018

jrick Oct 5, 2018

jrick Oct 5, 2018

vctt94 Oct 16, 2018

jrick Oct 17, 2018

jrick Oct 5, 2018

jrick Oct 5, 2018

vctt94 commented Oct 16, 2018

jrick Oct 17, 2018

jrick Oct 17, 2018

jrick Oct 17, 2018

jrick Oct 17, 2018

jrick Oct 17, 2018

jrick Oct 17, 2018

jrick Oct 17, 2018

jrick Oct 17, 2018

vctt94 Oct 24, 2018

jrick Oct 17, 2018

jrick Oct 17, 2018

vctt94 commented Oct 30, 2018

vctt94 commented Feb 26, 2019

jrick commented Mar 25, 2020

legacyrpc: add fundrawtransaction command #1195

legacyrpc: add fundrawtransaction command #1195

Conversation

vctt94 commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vctt94 commented Aug 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vctt94 commented Oct 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vctt94 commented Oct 30, 2018

vctt94 commented Feb 26, 2019

jrick commented Mar 25, 2020