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

client: bond rotation #2036

Merged
merged 8 commits into from Feb 8, 2023
26 changes: 12 additions & 14 deletions client/asset/dcr/dcr.go
Expand Up @@ -3256,9 +3256,9 @@ func (dcr *ExchangeWallet) EstimateRegistrationTxFee(feeRate uint64) uint64 {
// returned as the Data field of the Bond. The bond output pays to a pubkeyhash
// script for a wallet address. Bond.RedeemTx is a backup transaction that
// spends the bond output after lockTime passes, paying to an address for the
// current underlying wallet; the bond private key (BondPrivKey) should normally
// be used to author a new transaction paying to a new address instead.
func (dcr *ExchangeWallet) MakeBondTx(ver uint16, amt uint64, lockTime time.Time,
// current underlying wallet; the bond private key should normally be used to
// author a new transaction paying to a new address instead.
func (dcr *ExchangeWallet) MakeBondTx(ver uint16, amt, feeRate uint64, lockTime time.Time,
bondKey *secp256k1.PrivateKey, acctID []byte) (*asset.Bond, error) {
if ver != 0 {
return nil, errors.New("only version 0 bonds supported")
Expand All @@ -3272,8 +3272,7 @@ func (dcr *ExchangeWallet) MakeBondTx(ver uint16, amt uint64, lockTime time.Time
pk := bondKey.PubKey().SerializeCompressed()
pkh := stdaddr.Hash160(pk)

feeRate := dcr.targetFeeRateWithFallback(2, 0)

feeRate = dcr.feeRateWithFallback(feeRate)
baseTx := wire.NewMsgTx()
const scriptVersion = 0

Expand Down Expand Up @@ -3367,15 +3366,14 @@ func (dcr *ExchangeWallet) MakeBondTx(ver uint16, amt uint64, lockTime time.Time
}

bond := &asset.Bond{
Version: ver,
AssetID: BipID,
Amount: amt,
CoinID: toCoinID(&txid, 0),
Data: bondScript,
BondPrivKey: bondKey.Serialize(),
SignedTx: signedTxBytes,
UnsignedTx: unsignedTxBytes,
RedeemTx: redeemTx,
Version: ver,
AssetID: BipID,
Amount: amt,
CoinID: toCoinID(&txid, 0),
Data: bondScript,
SignedTx: signedTxBytes,
UnsignedTx: unsignedTxBytes,
RedeemTx: redeemTx,
}
success = true

Expand Down
22 changes: 11 additions & 11 deletions client/asset/dcr/rpcwallet.go
Expand Up @@ -525,6 +525,17 @@ func (w *rpcWallet) InternalAddress(ctx context.Context, acctName string) (stdad
return addr, translateRPCCancelErr(err)
}

// ExternalAddress returns an external address from the specified account using
// GapPolicyWrap. The dcrwallet user should set --gaplimit= as needed to prevent
// address reused depending on their needs. Part of the Wallet interface.
func (w *rpcWallet) ExternalAddress(ctx context.Context, acctName string) (stdaddr.Address, error) {
addr, err := w.rpcClient.GetNewAddressGapPolicy(ctx, acctName, dcrwallet.GapPolicyWrap)
Comment on lines +529 to +532
Copy link
Member Author

@chappjc chappjc Jan 16, 2023

Choose a reason for hiding this comment

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

As the revised comment states, with an external dcrwallet, the user can pick their --gaplimit as desired. Using ignore with the RPC has created difficult recover situations. The InternalAddress method already uses wrap implicitly (getrawchangeaddress does not even expose gap policy).
Separate commit.

Copy link
Member

Choose a reason for hiding this comment

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

Do you know if addresses that are known to be used are skipped when using GapPolicyWrap?

Copy link
Member Author

Choose a reason for hiding this comment

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

When an address is used, the next (unused) address index becomes the address to which wrapping returns. The wrap algo just ensures the gap limit isn't exceeded.

if err != nil {
return nil, translateRPCCancelErr(err)
}
return addr, nil
}

// LockUnspent locks or unlocks the specified outpoint.
// Part of the Wallet interface.
func (w *rpcWallet) LockUnspent(ctx context.Context, unlock bool, ops []*wire.OutPoint) error {
Expand Down Expand Up @@ -578,17 +589,6 @@ func (w *rpcWallet) UnspentOutput(ctx context.Context, txHash *chainhash.Hash, i
return nil, asset.CoinNotFoundError
}

// ExternalAddress returns an external address from the specified account using
// GapPolicyIgnore.
// Part of the Wallet interface.
func (w *rpcWallet) ExternalAddress(ctx context.Context, acctName string) (stdaddr.Address, error) {
addr, err := w.rpcClient.GetNewAddressGapPolicy(ctx, acctName, dcrwallet.GapPolicyIgnore)
if err != nil {
return nil, translateRPCCancelErr(err)
}
return addr, nil
}

// SignRawTransaction signs the provided transaction using rpc RawRequest.
// Part of the Wallet interface.
func (w *rpcWallet) SignRawTransaction(ctx context.Context, inTx *wire.MsgTx) (*wire.MsgTx, error) {
Expand Down
12 changes: 1 addition & 11 deletions client/asset/dcr/simnet_test.go
Expand Up @@ -178,7 +178,7 @@ func TestMakeBondTx(t *testing.T) {
}

lockTime := time.Now().Add(5 * time.Minute)
bond, err := wallet.MakeBondTx(bondVer, fee, lockTime, priv, acctID)
bond, err := wallet.MakeBondTx(bondVer, fee, 10, lockTime, priv, acctID)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -213,16 +213,6 @@ func TestMakeBondTx(t *testing.T) {
lockTimePush := time.Unix(int64(lockTimeUint), 0)
t.Logf("lock time in bond script: %v", lockTimePush)

privOut := secp256k1.PrivKeyFromBytes(bond.BondPrivKey)
pk := privOut.PubKey()
if !pk.IsEqual(pubkey) {
t.Fatalf("privkey does not match pubkey from bond script and bond signature")
}

if !privOut.Key.Equals(&priv.Key) {
t.Fatalf("serialized private key from asset.Bond does not match private key provided private key")
}

sendBondTx, err := wallet.SendTransaction(bond.SignedTx)
if err != nil {
t.Fatalf("RefundBond: %v", err)
Expand Down
2 changes: 1 addition & 1 deletion client/asset/dcr/spv.go
Expand Up @@ -546,7 +546,7 @@ func (w *spvWallet) ExternalAddress(ctx context.Context, _ string) (stdaddr.Addr
// InternalAddress returns an internal address using GapPolicyIgnore.
// Part of the Wallet interface.
func (w *spvWallet) InternalAddress(ctx context.Context, _ string) (stdaddr.Address, error) {
return w.NewInternalAddress(ctx, w.acctNum, wallet.WithGapPolicyIgnore())
Copy link
Member Author

@chappjc chappjc Jan 16, 2023

Choose a reason for hiding this comment

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

For the SPV wallet, match the external address method here. Plus I think we have 250 as the gap limit for our native wallet, although I haven't verified that. Native wallet recover with gap limit violations is much harder than external where the user just adds CLI switches, so let's avoid that at all costs.

return w.NewInternalAddress(ctx, w.acctNum, wallet.WithGapPolicyWrap())
}

// SignRawTransaction signs the provided transaction.
Expand Down
19 changes: 8 additions & 11 deletions client/asset/interface.go
Expand Up @@ -477,7 +477,7 @@ type Bonder interface {
// provided amount, lock time, and dex account ID. An explicit private key
// type is used to guarantee it's not bytes from something else like a
// public key.
MakeBondTx(ver uint16, amt uint64, lockTime time.Time, privKey *secp256k1.PrivateKey, acctID []byte) (*Bond, error)
MakeBondTx(ver uint16, amt, feeRate uint64, lockTime time.Time, privKey *secp256k1.PrivateKey, acctID []byte) (*Bond, error)
// RefundBond will refund the bond given the full bond output details and
// private key to spend it.
RefundBond(ctx context.Context, ver uint16, coinID, script []byte, amt uint64, privKey *secp256k1.PrivateKey) ([]byte, error)
Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I should mention that I've made RefundBond actually broadcast it internally in bond-reserves, rather than return the raw tx that gets send by core. I am positive that was suggested in the previous PR and I just neglected to apply the change. That pattern only needs to exist for MakeBondTx

Expand Down Expand Up @@ -745,16 +745,13 @@ type PeerManager interface {
// and lock time. These data are intended for the "post bond" request, in which
// the server pre-validates the unsigned transaction, the client then publishes
// the corresponding signed transaction, and a final request is made once the
// bond is fully confirmed. The bond key is kept in this struct to keep it
// coupled with the bond identity, and a redeem transaction is provided as a
// backup.
// bond is fully confirmed. The caller should manage the private key.
type Bond struct {
Version uint16
AssetID uint32
Amount uint64
CoinID []byte
Data []byte // additional data to interpret the bond e.g. redeem script, bond contract, etc.
BondPrivKey []byte // caller provided, but kept with the output
Version uint16
AssetID uint32
Amount uint64
CoinID []byte
Data []byte // additional data to interpret the bond e.g. redeem script, bond contract, etc.
// SignedTx and UnsignedTx are the opaque (raw bytes) signed and unsigned
// bond creation transactions, in whatever encoding and funding scheme for
// this asset and wallet. The unsigned one is used to pre-validate this bond
Expand All @@ -763,7 +760,7 @@ type Bond struct {
// published by the wallet.
SignedTx, UnsignedTx []byte
// RedeemTx is a backup transaction that spends the bond output. Normally
// the BondPrivKey will be used when the bond expires.
// the a key index will be used to derive the key when the bond expires.
RedeemTx []byte
}

Expand Down
4 changes: 2 additions & 2 deletions client/cmd/dexcctl/config.go
Expand Up @@ -58,7 +58,7 @@ func configure() (*config, []string, bool, error) {
cfg := &config{
Config: defaultConfigPath,
}
preParser := flags.NewParser(cfg, flags.HelpFlag)
preParser := flags.NewParser(cfg, flags.HelpFlag|flags.PassDoubleDash|flags.PassAfterNonOption)
_, err := preParser.Parse()
if err != nil {
var flagErr *flags.Error
Expand All @@ -85,7 +85,7 @@ func configure() (*config, []string, bool, error) {
return nil, nil, stop, nil
}

parser := flags.NewParser(cfg, flags.Default)
parser := flags.NewParser(cfg, flags.Default|flags.PassAfterNonOption)

if dex.FileExists(cfg.Config) {
// Load additional config from file.
Expand Down
4 changes: 2 additions & 2 deletions client/cmd/dexcctl/dexcctl_test.go
Expand Up @@ -75,12 +75,12 @@ func TestConfigure(t *testing.T) {
}

// parse args
os.Args = []string{"", "-C.nofile", "arg1", "arg2"}
os.Args = []string{"", "-C.nofile", "arg1", "arg2", "-1"}
_, args, _, err := configure()
if err != nil {
t.Fatal(err)
}
if args[0] != "arg1" && args[1] != "arg2" {
if args[0] != "arg1" || args[1] != "arg2" || args[2] != "-1" {
t.Fatal("arguments not parsed correctly")
}

Expand Down