bookkeeper: save last timestamp to avoid another query in find_consolidated_fees.
What the developer wrote
Authored by Rusty Russell
bookkeeper: save last timestamp to avoid another query in find_consolidated_fees.
If the fees are not *all* of the fees (as we do in next patch), the
query would be wrong. Plus, as the FIXME suggests, we should just save
it as we're getting the fee_sums, not do a whole new query!
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
What changed, and why it matters
This is a small internal cleanup in the bookkeeper plugin, which tracks on-chain Bitcoin fees for accounting. It removes a redundant database-style lookup by remembering the latest timestamp while fees are being added up. The commit message says this change is needed so a follow-up patch can correctly report fees that are not the complete set. There is no direct evidence this fixes an exploitable security bug.
Treat as a normal code-quality/correctness improvement. No urgent security action required. Review the follow-up patch referenced in the commit message to confirm the consolidated-fee logic behaves correctly for partial fee sets.
Security signals we found
Correctness fix for accounting timestamp derivation on filtered fee sets
Removal of redundant re-query that could return wrong results in future code paths
No input validation, buffer handling, cryptographic, or authorization changes observed
Evidence from the diff
The patch refactors fee summarization in plugins/bkpr. Previously, find_consolidated_fees() called onchain_fee_last_timestamp(), which re-queried all chain fees for an account and scanned for the matching txid to find the maximum timestamp. Now fee_sums_by_txid_and_account() tracks the maximum timestamp as it iterates over onchain_fee entries, storing it in a new fee_sum->last_timestamp field. The old helper and its FIXME are removed. The commit explicitly notes the old query would be wrong once fees are not all of the fees (i.e., a filtered/subset list). This is a correctness/robustness improvement for accounting output, not a memory-safety or remote-exploitable vulnerability fix.
Changed components
plugins/bkpr/incomestmt.cplugins/bkpr/onchain_fee.cplugins/bkpr/onchain_fee.hplugins/bkpr/recorder.hInspect captured patch +5 / −28
diff --git a/plugins/bkpr/incomestmt.c b/plugins/bkpr/incomestmt.c
index fd3a7e45..faf13552 100644
--- a/plugins/bkpr/incomestmt.c
+++ b/plugins/bkpr/incomestmt.c
@@ -290,11 +290,7 @@ static struct onchain_fee **find_consolidated_fees(const tal_t *ctx,
fee->debit = AMOUNT_MSAT(0);
fee->acct_name = tal_steal(fee, sums[i]->acct_name);
fee->txid = *sums[i]->txid;
-
- fee->timestamp =
- onchain_fee_last_timestamp(bkpr, sums[i]->acct_name,
- sums[i]->txid);
-
+ fee->timestamp = sums[i]->last_timestamp;
tal_arr_expand(&fee_sums, fee);
}
diff --git a/plugins/bkpr/onchain_fee.c b/plugins/bkpr/onchain_fee.c
index 4af85e25..0bec506b 100644
--- a/plugins/bkpr/onchain_fee.c
+++ b/plugins/bkpr/onchain_fee.c
@@ -366,10 +366,13 @@ static struct fee_sum **fee_sums_by_txid_and_account(const tal_t *ctx,
sum->txid = tal_dup(sum, struct bitcoin_txid,
&ofs[i]->txid);
credit = debit = AMOUNT_MSAT(0);
+ sum->last_timestamp = 0;
}
ok = amount_msat_accumulate(&credit, ofs[i]->credit);
assert(ok);
ok = amount_msat_accumulate(&debit, ofs[i]->debit);
+ if (ofs[i]->timestamp > sum->last_timestamp)
+ sum->last_timestamp = ofs[i]->timestamp;
}
/* Final, if any */
@@ -679,24 +682,6 @@ struct fee_sum **find_account_onchain_fees(const tal_t *ctx,
return fee_sums_by_txid_and_account(ctx, ofs);
}
-/* FIXME: Put this value into fee_sums! */
-u64 onchain_fee_last_timestamp(const struct bkpr *bkpr,
- const char *acct_name,
- const struct bitcoin_txid *txid)
-{
- struct onchain_fee **ofs;
- u64 timestamp = 0;
-
- ofs = account_get_chain_fees(tmpctx, bkpr, acct_name);
- for (size_t i = 0; i < tal_count(ofs); i++) {
- if (!bitcoin_txid_eq(&ofs[i]->txid, txid))
- continue;
- if (ofs[i]->timestamp > timestamp)
- timestamp = ofs[i]->timestamp;
- }
- return timestamp;
-}
-
/* If we're freeing the entire hash table, remove destructors from
* individual entries! */
static void ofees_hash_destroy(struct ofees_hash *ofees_hash)
diff --git a/plugins/bkpr/onchain_fee.h b/plugins/bkpr/onchain_fee.h
index 8b954b09..5e3be5e5 100644
--- a/plugins/bkpr/onchain_fee.h
+++ b/plugins/bkpr/onchain_fee.h
@@ -67,11 +67,6 @@ struct fee_sum **find_account_onchain_fees(const tal_t *ctx,
struct fee_sum **calculate_onchain_fee_sums(const tal_t *ctx,
const struct bkpr *bkpr);
-/* Find the last timestamp for the onchain fees for this txid + account */
-u64 onchain_fee_last_timestamp(const struct bkpr *bkpr,
- const char *acct_name,
- const struct bitcoin_txid *txid);
-
/* Update our onchain fees now? */
char *maybe_update_onchain_fees(const tal_t *ctx,
struct command *cmd,
diff --git a/plugins/bkpr/recorder.h b/plugins/bkpr/recorder.h
index 3c807a7e..c13c0f21 100644
--- a/plugins/bkpr/recorder.h
+++ b/plugins/bkpr/recorder.h
@@ -18,6 +18,7 @@ struct fee_sum {
const char *acct_name;
struct bitcoin_txid *txid;
struct amount_msat fees_paid;
+ u64 last_timestamp;
};
struct txo_pair {
Why this scored 16/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.