From e0623e7cddb568d1cc12ef5128e7b5164ff6829e Mon Sep 17 00:00:00 2001 From: ffranr Date: Thu, 19 Jan 2023 10:42:55 +0000 Subject: [PATCH 1/3] rpc: rename variable to clarify its purpose --- rpcserver.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/rpcserver.go b/rpcserver.go index 146f02a1f..f78222197 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -2261,9 +2261,10 @@ func (s *rpcServer) prepareLeasesResponse(ctx context.Context, totalAmtPaid btcutil.Amount ) for _, batch := range batches { - // Keep a tally of how many channels were created within each - // batch in order to estimate the chain fees paid. - numChans := 0 + // Keep a tally of the total number of channels which were + // created within each batch in order to estimate the chain fees + // paid. + totalNumChannels := 0 // Keep track of the index for the first lease found for this // batch. We'll use this to know where to start from when @@ -2415,7 +2416,8 @@ func (s *rpcServer) prepareLeasesResponse(ctx context.Context, SidecarChannel: isSidecarChannel, }) - numChans++ + // Count the total number of channels. + totalNumChannels++ } // Estimate the chain fees paid for the number of @@ -2427,7 +2429,7 @@ func (s *rpcServer) prepareLeasesResponse(ctx context.Context, // also take a look at the actual account version at the // time of the batch. chainFee := order.EstimateTraderFee( - uint32(numChans), batch.BatchTxFeeRate, + uint32(totalNumChannels), batch.BatchTxFeeRate, account.VersionInitialNoVersion, ) @@ -2435,8 +2437,10 @@ func (s *rpcServer) prepareLeasesResponse(ctx context.Context, // lease. Since multiple leases can be created within a // batch, we'll need to evenly distribute. We'll always // apply the remainder to the first lease in the batch. - chainFeePerChan := chainFee / btcutil.Amount(numChans) - chainFeePerChanRem := chainFee % btcutil.Amount(numChans) + chainFeePerChan := + chainFee / btcutil.Amount(totalNumChannels) + chainFeePerChanRem := + chainFee % btcutil.Amount(totalNumChannels) for i := firstIdxForBatch; i < len(rpcLeases); i++ { chainFee := chainFeePerChan if i == firstIdxForBatch { From 441d95c40ee1393134c669a4f98d35e24d42afea Mon Sep 17 00:00:00 2001 From: ffranr Date: Thu, 19 Jan 2023 14:04:23 +0000 Subject: [PATCH 2/3] clientdb: add snapshot of spending accounts to local batch snapshot The account versions from the spending accounts snapshot will be used in estimating on-chain fees. --- clientdb/batch.go | 7 +++++-- clientdb/batch_snapshot.go | 36 +++++++++++++++++++++++++++--------- 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/clientdb/batch.go b/clientdb/batch.go index a76fd94d4..d3aa8c9ba 100644 --- a/clientdb/batch.go +++ b/clientdb/batch.go @@ -105,6 +105,8 @@ func (db *DB) StorePendingBatch(batch *order.Batch, orders []order.Nonce, return err } + spendingAccountsSnapshot := accounts + var updatedAccounts []*account.Account for idx, acct := range accounts { accountKey := getAccountKey(acct) @@ -125,10 +127,11 @@ func (db *DB) StorePendingBatch(batch *order.Batch, orders []order.Nonce, return err } - // Before we are done, we store a snapshot of the this batch, + // Before we are done, we store a snapshot of the batch, // so we retain this history for later. snapshot, err := NewSnapshot( - batch, updatedOrders, updatedAccounts, + batch, updatedOrders, spendingAccountsSnapshot, + updatedAccounts, ) if err != nil { return err diff --git a/clientdb/batch_snapshot.go b/clientdb/batch_snapshot.go index d9bd9b0d7..3daf6ae95 100644 --- a/clientdb/batch_snapshot.go +++ b/clientdb/batch_snapshot.go @@ -82,6 +82,14 @@ type LocalBatchSnapshot struct { // the batch transaction. BatchTxFeeRate chainfee.SatPerKWeight + // spendingAccountsSnapshot is a snapshot of the local accounts that + // participated in this batch before any potential account updates were + // applied. + // + // NOTE: this field is used for estimating on-chains fee using the + // account version. + SpendingAccountsSnapshot map[[33]byte]*account.Account + // Account holds snapshots of the ending state of the local accounts // that participated in this batch. Accounts map[[33]byte]*account.Account @@ -97,6 +105,7 @@ type LocalBatchSnapshot struct { // NewSnapshot creates a new LocalBatchSnapshot from the passed order batched. func NewSnapshot(batch *order.Batch, ourOrders []order.Order, + spendingAccountsSnapshot []*account.Account, accounts []*account.Account) (*LocalBatchSnapshot, error) { // We only support LinearFeeSchedule at this point (because of @@ -107,6 +116,14 @@ func NewSnapshot(batch *order.Batch, ourOrders []order.Order, batch.ExecutionFee) } + // Reformulate account data structure into a map keyed on trader key. + spendingAccsSnapshot := make(map[[33]byte]*account.Account) + for _, a := range accounts { + var key [33]byte + copy(key[:], a.TraderKey.PubKey.SerializeCompressed()) + spendingAccsSnapshot[key] = a + } + as := make(map[[33]byte]*account.Account) for _, a := range accounts { var key [33]byte @@ -120,15 +137,16 @@ func NewSnapshot(batch *order.Batch, ourOrders []order.Order, } snapshot := &LocalBatchSnapshot{ - Version: batch.Version, - BatchID: batch.ID, - ClearingPrices: batch.ClearingPrices, - ExecutionFee: *feeSched, - BatchTX: batch.BatchTX, - BatchTxFeeRate: batch.BatchTxFeeRate, - Accounts: as, - Orders: os, - MatchedOrders: batch.MatchedOrders, + Version: batch.Version, + BatchID: batch.ID, + ClearingPrices: batch.ClearingPrices, + ExecutionFee: *feeSched, + BatchTX: batch.BatchTX, + BatchTxFeeRate: batch.BatchTxFeeRate, + SpendingAccountsSnapshot: spendingAccsSnapshot, + Accounts: as, + Orders: os, + MatchedOrders: batch.MatchedOrders, } return snapshot, nil From 1a14e73f220d989bc3056e3ea9ddc60ee54d4bb1 Mon Sep 17 00:00:00 2001 From: ffranr Date: Thu, 19 Jan 2023 11:27:13 +0000 Subject: [PATCH 3/3] rpc: batch on-chain fee estimate calc includes account version Account version is taken from a snapshot of spending account states. This is because the account might be updated to a new version within the batch. --- rpcserver.go | 39 ++++++++++++++++++++++++++++----------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/rpcserver.go b/rpcserver.go index f78222197..88e73f497 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -2266,6 +2266,9 @@ func (s *rpcServer) prepareLeasesResponse(ctx context.Context, // paid. totalNumChannels := 0 + // Keep a tally of the total number of channels per account key. + accountChannelsCount := make(map[[33]byte]uint32) + // Keep track of the index for the first lease found for this // batch. We'll use this to know where to start from when // populating the ChainFeeSat field for each lease. @@ -2282,7 +2285,8 @@ func (s *rpcServer) prepareLeasesResponse(ctx context.Context, } // Filter out the account if required. - _, ok = accounts[ourOrder.Details().AcctKey] + accountKey := ourOrder.Details().AcctKey + _, ok = accounts[accountKey] if len(accounts) != 0 && !ok { continue } @@ -2418,20 +2422,33 @@ func (s *rpcServer) prepareLeasesResponse(ctx context.Context, // Count the total number of channels. totalNumChannels++ + + // Count the total number of channels per + // account. + accountChannelsCount[accountKey]++ } // Estimate the chain fees paid for the number of // channels created in this batch and tally them. - // - // TODO(guggero): This is just an approximation! We - // should properly calculate the fees _per account_ as - // that's what we do on the server side. Then we can - // also take a look at the actual account version at the - // time of the batch. - chainFee := order.EstimateTraderFee( - uint32(totalNumChannels), batch.BatchTxFeeRate, - account.VersionInitialNoVersion, - ) + chainFee := btcutil.Amount(0) + for accountKey, numChannels := range accountChannelsCount { + // Calculate and sum the account specific chain + // fee estimate. + // + // The account version influences the chain fee. + // It is important to use the account version + // that was used when the batch was created. + // Accounts might be updated within the batch. + // Therefore, we query the account version from + // an account spending snapshot. + spendingAcctSnapshot := + batch.SpendingAccountsSnapshot[accountKey] + + chainFee += order.EstimateTraderFee( + numChannels, batch.BatchTxFeeRate, + spendingAcctSnapshot.Version, + ) + } // We'll need to compute the chain fee paid for each // lease. Since multiple leases can be created within a