From 15a1b79a1b384b84bbd6869ac29d5360d6b25a0c Mon Sep 17 00:00:00 2001 From: positiveblue Date: Thu, 23 Sep 2021 19:56:39 -0700 Subject: [PATCH 1/3] poolrpc: add optional raw sat/byte parameter to InitAccountRequest --- poolrpc/trader.pb.go | 25 +++++++++++++++++++++++-- poolrpc/trader.proto | 7 +++++-- poolrpc/trader.swagger.json | 5 +++++ 3 files changed, 33 insertions(+), 4 deletions(-) diff --git a/poolrpc/trader.pb.go b/poolrpc/trader.pb.go index a3fd7a2cd..b6bbf7e2d 100644 --- a/poolrpc/trader.pb.go +++ b/poolrpc/trader.pb.go @@ -267,6 +267,7 @@ type InitAccountRequest struct { AccountExpiry isInitAccountRequest_AccountExpiry `protobuf_oneof:"account_expiry"` // Types that are assignable to Fees: // *InitAccountRequest_ConfTarget + // *InitAccountRequest_FeeRateSatPerKw Fees isInitAccountRequest_Fees `protobuf_oneof:"fees"` // //An optional identification string that will be appended to the user agent @@ -351,6 +352,13 @@ func (x *InitAccountRequest) GetConfTarget() uint32 { return 0 } +func (x *InitAccountRequest) GetFeeRateSatPerKw() uint64 { + if x, ok := x.GetFees().(*InitAccountRequest_FeeRateSatPerKw); ok { + return x.FeeRateSatPerKw + } + return 0 +} + func (x *InitAccountRequest) GetInitiator() string { if x != nil { return x.Initiator @@ -384,8 +392,17 @@ type InitAccountRequest_ConfTarget struct { ConfTarget uint32 `protobuf:"varint,4,opt,name=conf_target,json=confTarget,proto3,oneof"` } +type InitAccountRequest_FeeRateSatPerKw struct { + // + //The fee rate, in satoshis per kw, to use for the initial funding + //transaction. + FeeRateSatPerKw uint64 `protobuf:"varint,6,opt,name=fee_rate_sat_per_kw,json=feeRateSatPerKw,proto3,oneof"` +} + func (*InitAccountRequest_ConfTarget) isInitAccountRequest_Fees() {} +func (*InitAccountRequest_FeeRateSatPerKw) isInitAccountRequest_Fees() {} + type QuoteAccountRequest struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache @@ -4702,7 +4719,7 @@ var file_trader_proto_rawDesc = []byte{ 0x0a, 0x0c, 0x74, 0x72, 0x61, 0x64, 0x65, 0x72, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x12, 0x07, 0x70, 0x6f, 0x6f, 0x6c, 0x72, 0x70, 0x63, 0x1a, 0x1e, 0x61, 0x75, 0x63, 0x74, 0x69, 0x6f, 0x6e, 0x65, 0x65, 0x72, 0x72, 0x70, 0x63, 0x2f, 0x61, 0x75, 0x63, 0x74, 0x69, 0x6f, 0x6e, 0x65, 0x65, - 0x72, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x22, 0xea, 0x01, 0x0a, 0x12, 0x49, 0x6e, 0x69, 0x74, + 0x72, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x22, 0x9a, 0x02, 0x0a, 0x12, 0x49, 0x6e, 0x69, 0x74, 0x41, 0x63, 0x63, 0x6f, 0x75, 0x6e, 0x74, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x12, 0x23, 0x0a, 0x0d, 0x61, 0x63, 0x63, 0x6f, 0x75, 0x6e, 0x74, 0x5f, 0x76, 0x61, 0x6c, 0x75, 0x65, 0x18, 0x01, 0x20, 0x01, 0x28, 0x04, 0x52, 0x0c, 0x61, 0x63, 0x63, 0x6f, 0x75, 0x6e, 0x74, 0x56, 0x61, @@ -4713,7 +4730,10 @@ var file_trader_proto_rawDesc = []byte{ 0x74, 0x18, 0x03, 0x20, 0x01, 0x28, 0x0d, 0x48, 0x00, 0x52, 0x0e, 0x72, 0x65, 0x6c, 0x61, 0x74, 0x69, 0x76, 0x65, 0x48, 0x65, 0x69, 0x67, 0x68, 0x74, 0x12, 0x21, 0x0a, 0x0b, 0x63, 0x6f, 0x6e, 0x66, 0x5f, 0x74, 0x61, 0x72, 0x67, 0x65, 0x74, 0x18, 0x04, 0x20, 0x01, 0x28, 0x0d, 0x48, 0x01, - 0x52, 0x0a, 0x63, 0x6f, 0x6e, 0x66, 0x54, 0x61, 0x72, 0x67, 0x65, 0x74, 0x12, 0x1c, 0x0a, 0x09, + 0x52, 0x0a, 0x63, 0x6f, 0x6e, 0x66, 0x54, 0x61, 0x72, 0x67, 0x65, 0x74, 0x12, 0x2e, 0x0a, 0x13, + 0x66, 0x65, 0x65, 0x5f, 0x72, 0x61, 0x74, 0x65, 0x5f, 0x73, 0x61, 0x74, 0x5f, 0x70, 0x65, 0x72, + 0x5f, 0x6b, 0x77, 0x18, 0x06, 0x20, 0x01, 0x28, 0x04, 0x48, 0x01, 0x52, 0x0f, 0x66, 0x65, 0x65, + 0x52, 0x61, 0x74, 0x65, 0x53, 0x61, 0x74, 0x50, 0x65, 0x72, 0x4b, 0x77, 0x12, 0x1c, 0x0a, 0x09, 0x69, 0x6e, 0x69, 0x74, 0x69, 0x61, 0x74, 0x6f, 0x72, 0x18, 0x05, 0x20, 0x01, 0x28, 0x09, 0x52, 0x09, 0x69, 0x6e, 0x69, 0x74, 0x69, 0x61, 0x74, 0x6f, 0x72, 0x42, 0x10, 0x0a, 0x0e, 0x61, 0x63, 0x63, 0x6f, 0x75, 0x6e, 0x74, 0x5f, 0x65, 0x78, 0x70, 0x69, 0x72, 0x79, 0x42, 0x06, 0x0a, 0x04, @@ -6418,6 +6438,7 @@ func file_trader_proto_init() { (*InitAccountRequest_AbsoluteHeight)(nil), (*InitAccountRequest_RelativeHeight)(nil), (*InitAccountRequest_ConfTarget)(nil), + (*InitAccountRequest_FeeRateSatPerKw)(nil), } file_trader_proto_msgTypes[1].OneofWrappers = []interface{}{ (*QuoteAccountRequest_ConfTarget)(nil), diff --git a/poolrpc/trader.proto b/poolrpc/trader.proto index e81f7e43f..beb4a3d96 100644 --- a/poolrpc/trader.proto +++ b/poolrpc/trader.proto @@ -221,8 +221,11 @@ message InitAccountRequest { */ uint32 conf_target = 4; - // TODO(guggero): Add sat_per_vbyte as soon as lnd has a parameter for - // that in rpcServer.EstimateFee. + /* + The fee rate, in satoshis per kw, to use for the initial funding + transaction. + */ + uint64 fee_rate_sat_per_kw = 6; } /* diff --git a/poolrpc/trader.swagger.json b/poolrpc/trader.swagger.json index 3da912607..541627795 100644 --- a/poolrpc/trader.swagger.json +++ b/poolrpc/trader.swagger.json @@ -1495,6 +1495,11 @@ "format": "int64", "description": "The target number of blocks that the transaction should be confirmed in." }, + "fee_rate_sat_per_kw": { + "type": "string", + "format": "uint64", + "description": "The fee rate, in satoshis per kw, to use for the initial funding\ntransaction." + }, "initiator": { "type": "string", "description": "An optional identification string that will be appended to the user agent\nstring sent to the server to give information about the usage of pool. This\ninitiator part is meant for user interfaces to add their name to give the\nfull picture of the binary used (poold, LiT) and the method used for opening\nthe account (pool CLI, LiT UI, other 3rd party UI)." From 68a38b34d76c28c65af28eba29a60837df0fccdd Mon Sep 17 00:00:00 2001 From: positiveblue Date: Thu, 23 Sep 2021 20:23:22 -0700 Subject: [PATCH 2/3] account: use raw sat/byte when making an account Generally the confirmation target is much less granular than a direct sat/byte due to jumps in the confirmation confidence intervals. Changed the account manager to take the `feeRate` instead of the `confTarget`. The caller will be the one with the responsability of calculating the `freeRate`. If the caller does not provide a value (`freeRate=0`) we will calculate it from the default confTarget (6 blocks). --- account/manager.go | 69 ++++++++++++++++++----------------------- account/manager_test.go | 6 ++-- rpcserver.go | 50 ++++++++++++++++++++++++----- 3 files changed, 76 insertions(+), 49 deletions(-) diff --git a/account/manager.go b/account/manager.go index 911bc7378..8652ca70b 100644 --- a/account/manager.go +++ b/account/manager.go @@ -188,16 +188,26 @@ func (m *Manager) start() error { if err != nil { return fmt.Errorf("unable to retrieve accounts: %v", err) } + + // We calculate the default fee rate that will be used + // for resuming accounts for which we haven't created and broadcast + // a transaction yet + feeRate, err := m.cfg.Wallet.EstimateFee( + ctx, int32(DefaultFundingConfTarget), + ) + if err != nil { + return fmt.Errorf("unable to estimate default fees %w", err) + } + for _, account := range accounts { // Try to resume the account now. // // TODO(guggero): Refactor this to extract the init/funding // part so we properly abandon the account if it fails before // publishing the TX instead of trying to re-fund on startup. - err := m.resumeAccount( - ctx, account, true, false, DefaultFundingConfTarget, - ) - if err != nil { + if err := m.resumeAccount( + ctx, account, true, false, feeRate, + ); err != nil { return fmt.Errorf("unable to resume account %x: %v", account.TraderKey.PubKey.SerializeCompressed(), err) @@ -258,7 +268,8 @@ func (m *Manager) QuoteAccount(ctx context.Context, value btcutil.Amount, // InitAccount handles a request to create a new account with the provided // parameters. func (m *Manager) InitAccount(ctx context.Context, value btcutil.Amount, - expiry, bestHeight, confTarget uint32) (*Account, error) { + feeRate chainfee.SatPerKWeight, expiry, + bestHeight uint32) (*Account, error) { // We'll make sure to acquire the reservation lock throughout the // account funding process to ensure we use the same reservation, as @@ -281,26 +292,6 @@ func (m *Manager) InitAccount(ctx context.Context, value btcutil.Amount, return nil, err } - // Let's make sure our wallet contains enough coins to fund the account - // before we reserve any resources. We ask lnd to create a transaction - // to send the given account value in dry-run mode. This makes sure we - // actually have some UTXOs to fund the account as this method returns - // an error on insufficient balance. Unfortunately it currently only - // supports estimating the total fee for a transaction using a - // confirmation target. We'll want to add sats/vByte as well as soon as - // the API allows it. - totalMinerFee, err := m.cfg.TxFeeEstimator.EstimateFeeToP2WSH( - ctx, value, int32(confTarget), - ) - if err != nil { - return nil, fmt.Errorf("error estimating on-chain fee: %v", err) - } - - // A by-product of the balance check is the total fee we'd need to pay - // so we might as well log it here. - log.Infof("Estimated total chain fee of %v for new account with "+ - "value=%v, conf_target=%v", totalMinerFee, value, confTarget) - // We'll start by deriving a key for ourselves that we'll use in our // 2-of-2 multi-sig construction. keyDesc, err := m.cfg.Wallet.DeriveNextKey( @@ -350,7 +341,7 @@ func (m *Manager) InitAccount(ctx context.Context, value btcutil.Amount, log.Infof("Creating new account %x of %v that expires at height %v", keyDesc.PubKey.SerializeCompressed(), value, expiry) - err = m.resumeAccount(ctx, account, false, false, confTarget) + err = m.resumeAccount(ctx, account, false, false, feeRate) if err != nil { return nil, err } @@ -389,6 +380,8 @@ func (m *Manager) WatchMatchedAccounts(ctx context.Context, // closed because it was used up or pending batch update because // it was recreated. Either way, let's resume it now by creating // the appropriate watchers again. + // We set feerate to 0 because we know that we won't need to + // create a new transaction for resuming the account. err = m.resumeAccount(ctx, acct, false, false, 0) if err != nil { return fmt.Errorf("error resuming account %x: %v", @@ -418,7 +411,7 @@ func (m *Manager) maybeBroadcastTx(ctx context.Context, tx *wire.MsgTx, // This method serves as a way to consolidate the logic of resuming accounts on // startup and during normal operation. func (m *Manager) resumeAccount(ctx context.Context, account *Account, // nolint - onRestart bool, onRecovery bool, fundingConfTarget uint32) error { + onRestart bool, onRecovery bool, feeRate chainfee.SatPerKWeight) error { accountOutput, err := account.Output() if err != nil { @@ -480,26 +473,24 @@ func (m *Manager) resumeAccount(ctx context.Context, account *Account, // nolint } if createTx { - // We need a static sat/vByte value for the fee in - // SendOutputs, so we use the stored targetConf of the - // account to estimate it. - feeSatPerKw, err := m.cfg.Wallet.EstimateFee( - ctx, int32(fundingConfTarget), - ) - if err != nil { - return err + acctKey := account.TraderKey.PubKey.SerializeCompressed() + + if feeRate == 0 { + return fmt.Errorf("unable to create "+ + " transaction for account with "+ + " trader key %x, feeRate should "+ + "be greater than 0", acctKey) } // If we have a label prefix, then we'll apply that now // and also attach some additional meta data. - acctKey := account.TraderKey.PubKey.SerializeCompressed() contextLabel := fmt.Sprintf(" poold -- "+ "AccountCreation(acct_key=%x)", acctKey) label := makeTxnLabel(m.cfg.TxLabelPrefix, contextLabel) // TODO(wilmer): Expose manual controls to bump fees. tx, err := m.cfg.Wallet.SendOutputs( - ctx, []*wire.TxOut{accountOutput}, feeSatPerKw, + ctx, []*wire.TxOut{accountOutput}, feeRate, label, ) if err != nil { @@ -969,7 +960,7 @@ func (m *Manager) handleAccountSpend(traderKey *btcec.PublicKey, if ok { // Proceed with the rest of the flow. We won't send to // the account output again, so we don't need to set - // a valid conf target. + // a valid feeRate. return m.resumeAccount( context.Background(), account, false, false, 0, ) @@ -1464,7 +1455,7 @@ func (m *Manager) RecoverAccount(ctx context.Context, account *Account) error { // the `onRestart` flag to false because that would try to re-publish // the opening transaction in some cases which we don't want. Instead we // set the `onRecovery` flag to true. We won't send to the account - // output again, so we don't need to set a valid funding conf target. + // output again, so we don't need to set a valid funding freeRate. return m.resumeAccount(ctx, account, false, true, 0) } diff --git a/account/manager_test.go b/account/manager_test.go index 8cf062a02..d50b5aadf 100644 --- a/account/manager_test.go +++ b/account/manager_test.go @@ -151,7 +151,7 @@ func (h *testHarness) openAccount(value btcutil.Amount, expiry uint32, // nolint // Create a new account. Its initial state should be StatePendingOpen. ctx := context.Background() account, err := h.manager.InitAccount( - ctx, value, expiry, bestHeight, DefaultFundingConfTarget, + ctx, value, chainfee.FeePerKwFloor, expiry, bestHeight, ) if err != nil { h.t.Fatalf("unable to create new account: %v", err) @@ -531,8 +531,8 @@ func TestResumeAccountAfterRestart(t *testing.T) { // persisted intent to create the account. go func() { _, _ = h.manager.InitAccount( - context.Background(), value, expiry, bestHeight, - DefaultFundingConfTarget, + context.Background(), value, chainfee.FeePerKwFloor, expiry, + bestHeight, ) }() diff --git a/rpcserver.go b/rpcserver.go index 653f5d36c..8881a7fd4 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -510,17 +510,53 @@ func (s *rpcServer) InitAccount(ctx context.Context, "must be specified") } - // Determine the desired transaction fee. - confTarget := req.GetConfTarget() - if confTarget < 1 { - return nil, fmt.Errorf("confirmation target must be " + - "greater than 0") + if req.GetFeeRateSatPerKw() > 0 && req.GetConfTarget() > 0 { + return nil, fmt.Errorf("you must set only one of the sats/kw " + + "and confirmation target parameters") + } + + var feeRate chainfee.SatPerKWeight + + switch { + case req.GetFeeRateSatPerKw() > 0: + feeRate = chainfee.SatPerKWeight(req.GetFeeRateSatPerKw()) + + case req.GetConfTarget() > 0: + // Determine the desired transaction fee. + value := btcutil.Amount(req.AccountValue) + confTarget := req.GetConfTarget() + + feeRate, _, err := s.accountManager.QuoteAccount( + ctx, value, confTarget, + ) + if err != nil { + return nil, fmt.Errorf("unable to estimate on-chain fees: "+ + "%v", err) + } + + // If the fee estimation ever returns a value too small + // we set it to a valid minimum + if feeRate < chainfee.FeePerKwFloor { + feeRate = chainfee.FeePerKwFloor + } + + log.Infof("Estimated total chain fee of %v for new account with "+ + "value=%v, conf_target=%v", feeRate, value, confTarget) + + default: + return nil, fmt.Errorf("either sats/kw or confirmation target " + + "must be specified") + } + + if feeRate < chainfee.FeePerKwFloor { + return nil, fmt.Errorf("fee rate of %d sat/kw is too low, "+ + "minimum is %d sat/kw", feeRate, chainfee.FeePerKwFloor) } acct, err := s.accountManager.InitAccount( ContextWithInitiator(ctx, req.Initiator), - btcutil.Amount(req.AccountValue), expiryHeight, bestHeight, - confTarget, + btcutil.Amount(req.AccountValue), feeRate, + expiryHeight, bestHeight, ) if err != nil { return nil, err From c90f15878def13896fe4ee78bcc2a98b285a4b20 Mon Sep 17 00:00:00 2001 From: positiveblue Date: Wed, 29 Sep 2021 21:05:28 -0700 Subject: [PATCH 3/3] cmd/pool: add `sats_per_vbyte` to account new --- cmd/pool/account.go | 73 +++++++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 15 deletions(-) diff --git a/cmd/pool/account.go b/cmd/pool/account.go index 21b3e3947..835c57241 100644 --- a/cmd/pool/account.go +++ b/cmd/pool/account.go @@ -101,6 +101,11 @@ var newAccountCommand = cli.Command{ "within", Value: defaultFundingConfTarget, }, + cli.Uint64Flag{ + Name: "sat_per_vbyte", + Usage: "the fee rate expressed in sat/vbyte that " + + "should be used for the account creation transaction", + }, cli.BoolFlag{ Name: "force", Usage: "skip account fee confirmation", @@ -119,10 +124,37 @@ func newAccount(ctx *cli.Context) error { req := &poolrpc.InitAccountRequest{ AccountValue: amt, - Fees: &poolrpc.InitAccountRequest_ConfTarget{ + Initiator: defaultInitiator, + } + + satPerVByte := ctx.Uint64("sat_per_vbyte") + confTarget := ctx.Uint64("conf_target") + + switch { + case satPerVByte > 0: + // Enforce a minimum fee rate of 253 sat/kw by rounding up if 1 + // sat/byte is used. + feeRate := chainfee.SatPerKVByte(satPerVByte * 1000).FeePerKWeight() + if feeRate < chainfee.FeePerKwFloor { + feeRate = chainfee.FeePerKwFloor + } + req.Fees = &poolrpc.InitAccountRequest_FeeRateSatPerKw{ + FeeRateSatPerKw: uint64(feeRate), + } + + case ctx.IsSet("conf_target"): + if confTarget == 0 { + return fmt.Errorf("specified confirmation target must be " + + "greater than 0") + } + + req.Fees = &poolrpc.InitAccountRequest_ConfTarget{ ConfTarget: uint32(ctx.Uint64("conf_target")), - }, - Initiator: defaultInitiator, + } + + default: + return fmt.Errorf("either sat/vbyte or confirmation target " + + "must be specified") } // Parse the expiry in either of its forms. We'll always prefer the @@ -150,8 +182,10 @@ func newAccount(ctx *cli.Context) error { // present it to the user. if !ctx.Bool("force") { if err := printAccountFees( - client, btcutil.Amount(amt), - uint32(ctx.Uint64("conf_target")), + client, + btcutil.Amount(amt), + satPerVByte, + uint32(confTarget), ); err != nil { return err } @@ -173,27 +207,36 @@ func newAccount(ctx *cli.Context) error { } func printAccountFees(client poolrpc.TraderClient, amt btcutil.Amount, - confTarget uint32) error { + satPerVByte uint64, confTarget uint32) error { - req := &poolrpc.QuoteAccountRequest{ - AccountValue: uint64(amt), - Fees: &poolrpc.QuoteAccountRequest_ConfTarget{ - ConfTarget: confTarget, - }, + if confTarget == 0 { + fmt.Println("-- Account Funding Details --") + fmt.Printf("Amount: %v\n", amt) + fmt.Printf("Fee rate (estimated): %d sat/vByte\n", satPerVByte) + + return nil } - resp, err := client.QuoteAccount(context.Background(), req) + + resp, err := client.QuoteAccount( + context.Background(), + &poolrpc.QuoteAccountRequest{ + AccountValue: uint64(amt), + Fees: &poolrpc.QuoteAccountRequest_ConfTarget{ + ConfTarget: confTarget, + }, + }, + ) if err != nil { return fmt.Errorf("unable to estimate on-chain fees: "+ "%v", err) } feeRate := chainfee.SatPerKWeight(resp.MinerFeeRateSatPerKw) - satPerVByte := float64(feeRate.FeePerKVByte()) / 1000 - + feePerVByte := float64(feeRate.FeePerKVByte()) / 1000 fmt.Println("-- Account Funding Details --") fmt.Printf("Amount: %v\n", amt) fmt.Printf("Confirmation target: %v blocks\n", confTarget) - fmt.Printf("Fee rate (estimated): %.1f sat/vByte\n", satPerVByte) + fmt.Printf("Fee rate (estimated): %.1f sat/vByte\n", feePerVByte) fmt.Printf("Total miner fee (estimated): %v\n", btcutil.Amount(resp.MinerFeeTotal))