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

Fix onchain fees lookup query #6939

Conversation

evansmj
Copy link

@evansmj evansmj commented Dec 13, 2023

issue #6800

Grouping by update_count resulted in a crash due to a bad arithmetic assert in recorder.c line 365 calculating negative fees caused by the returned rows not being consolidated. This removes that update_count grouping and adds tests for single and dual funded channels.


Update

update_count is just the count of the records for a tx. To calculate onchain fees
for an account we must sum all credits vs debits. We don't need to GROUP BY update_count
nor ORDER BY update_count since it is just a running index of updates to this tx, so it can be removed.

Previously...
Postgres was not happy with not having update_count in the GROUP BY if it was used in ORDER BY.
The two pull requests below added update_count to this query for postgres:
#6141
#6128

However that resulted in a bad query result in certain situations as referenced in [issue #6800 (https://github.com//issues/6800)

the bad query resulted in something like this:

row  txid  credit  debit
1    blob  1000167000  0
2    blob  0  1000167000

which results in a failed assert due to bad arithmetic (0 - 1000167000 = negative number for a fee):

		ok = amount_msat_sub(&sum->fees_paid, sum->fees_paid, amt);
		assert(ok);

and would runtime crash bookkeeper inspect().

now removing update_count (what this pr originally did) from GROUP BY (but leaving it in ORDER BY) results in something like this:

row  txid  credit  debit
1    blob 1000167000 1000167000

and works for sqlite3 which is less strict on the sql standard. However the postgres tests were failing for the above reason.

Instead of adding update_count to the SELECT column, I remove it from this query entirely. Based on this, I don't believe we need it.

@evansmj evansmj force-pushed the evansmj/bugfix-find-onchain-fees-bkpr-inspect branch from f882412 to 3d843ed Compare December 14, 2023 15:18
tests/test_bookkeeper.py Outdated Show resolved Hide resolved
tests/test_bookkeeper.py Outdated Show resolved Hide resolved
@evansmj evansmj force-pushed the evansmj/bugfix-find-onchain-fees-bkpr-inspect branch from 10706ad to a82cf26 Compare December 20, 2023 14:36
@evansmj evansmj force-pushed the evansmj/bugfix-find-onchain-fees-bkpr-inspect branch 2 times, most recently from f83fe2f to 6610ce1 Compare January 16, 2024 18:16
Add test for single funded channels.
Add test for dual funded channels.
Remove grouping by update_count which resulted in a crash due to bad arithmetic
caused by fee calculation returned rows not being consolidated.
Remove xfail.
update_count is just the count of the records for a tx.  To calculate onchain fees
for an account we must sum all credits vs debits.  We don't need to GROUP BY update_count
nor ORDER BY update_count since it is just a running index of updates to this tx.
@cdecker cdecker force-pushed the evansmj/bugfix-find-onchain-fees-bkpr-inspect branch from 6610ce1 to 7c1f26c Compare February 2, 2024 15:20
@cdecker
Copy link
Member

cdecker commented Feb 2, 2024

ACK 7c1f26c

@cdecker cdecker enabled auto-merge (rebase) February 2, 2024 15:21
@cdecker cdecker merged commit 5544911 into ElementsProject:master Feb 2, 2024
32 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants