Only fetch onchain transactions from LND within start_time and end_time range #202
Conversation
nordbjorn
force-pushed
the
feat/onchain-pagination-temp
branch
from
e0d6aac to
74518d2
Compare
accounting/config.go
Outdated
| @@ -109,6 +111,58 @@ func NewOnChainConfig(ctx context.Context, lnd lndclient.LndServices, startTime, | |||
| } | |||
| } | |||
|
|
|||
| // Start and end height can initially be set to 0, meaning we will query | |||
| // for all onchain history | |||
There was a problem hiding this comment.
I know this isn't specifically mentioned in this repository, but we try to adhere to the same formatting rules in all our projects as we do in lnd: https://github.com/lightningnetwork/lnd/blob/master/docs/code_formatting_rules.md
So specifically things like always ending sentences with punctuation, wrapping code at 80 characters (while configuring the editor to use 8 spaces for the tab character) and so on.
There was a problem hiding this comment.
Thanks for the review and feedback!
I reviewed this doc and updated this PR to try and ensure it abides by these rules.
I also squashed the git history.
accounting/config.go
Outdated
| startHeight, err = blockHeightLookup(startTime) | ||
| if err != nil { | ||
| log.Errorf("Error finding block height based on startTime: %v "+ | ||
| "error: %v", startTime, err) | ||
|
|
||
| // If we cannot find the block height based on the start time, | ||
| // start looking from the genesis block. | ||
| startHeight = 0 | ||
| } else { | ||
| // Subtract 3 blocks just to be sure we do not grab too few blocks | ||
| // and miss any transaction | ||
| // (with safe handling for uint32 underflow) | ||
| startHeight = uint32(max(0, int64(startHeight)-3)) | ||
| } | ||
|
|
||
| endHeight, err = blockHeightLookup(endTime) | ||
| if err != nil { | ||
| log.Errorf("Error finding block height based on endHeight: %v "+ | ||
| "error: %v", endHeight, err) | ||
|
|
||
| // If we cannot find the block height based on the end time, | ||
| // set both start and end height to 0, meaning we will query for | ||
| // all onchain history. | ||
| startHeight = 0 | ||
| endHeight = 0 | ||
| } else if endHeight != 0 { | ||
| // Add 3 blocks just to be sure we do not grab too few blocks | ||
| // and miss any transaction | ||
| endHeight += 3 | ||
| } | ||
|
|
||
| if startHeight > endHeight { | ||
| log.Errorf("Start height: %v is greater than end height: %v, "+ | ||
| "setting both to 0", startHeight, endHeight) | ||
| startHeight = 0 | ||
| endHeight = 0 | ||
| } |
There was a problem hiding this comment.
Sounds like we could extract this into a function as well, that's then passed in as blockRangeLookup func(start, end time.Time) (uint32, uint32, error),.
There was a problem hiding this comment.
Good idea. I refactored according to this now.
I also realized that the buffer of 3 blocks in either direction might not be enough in all circumstances. Since the rules for the block timestamp are somewhat loose, I think this could result in potentially fetching too few blocks from the backend. So I updated the logic here so that we apply a 24h buffer on the timestamps when resolving to a block height. I think that should be enough even in extreme network conditions. What do you think?
frdrpcserver/node_audit.go
Outdated
| timeRangeSet := req.StartTime > 0 || req.EndTime > 0 | ||
| if timeRangeSet { | ||
| blockHeightLookup = func(targetTimestamp time.Time) (uint32, error) { | ||
| return findFirstBlockBeforeTimestamp(ctx, cfg.Lnd, info.BlockHeight, |
There was a problem hiding this comment.
nit: see formatting guideline for wrapping long function calls.
There was a problem hiding this comment.
Thanks, this is also fixed now
nordbjorn
force-pushed
the
feat/onchain-pagination-temp
branch
from
a01826c to
901eff5
Compare
| // certain range, we will apply a buffer on the time which we use to | ||
| // find the block height. This should ensure we use a low enough | ||
| // height to fetch all relevant transactions following the start time. | ||
| bufferedStartTime := startTime.Add(time.Hour * -24) |
There was a problem hiding this comment.
nit: probably would make sense to extract this range value into a named variable at the start of the file.
Ensure that the
NodeAuditRPC makes use of the providedstart_timeandend_timearguments, in the sense that Faraday will only query the LND backend for onchain transactions using a block height range which corresponds to thestart_timeandend_time(with a buffer).Motivation and Context
The
NodeAuditrequest can currently fail for old LND nodes, with a lot of historical onchain activity. Even if the caller provides a small time range to produce the report for. This is because currently all onchain transactions are fetched from the LND backend, and then filtered in memory based on the given time range.This PR aims to alleviate most of the problem by ensuring Faraday does not need to query the entire history of onchain transactions when the caller provides a time range for the report.
Related issue
#177
Implementation Notes
I did not find any obvious way to resolve a timestamp to a block height, which needs to be provided for the queries to the LND backend. For now I chose an approach where we do a binary search on the entire blockchain to find the block height just before the provided timestamp.
This means we need to look up several block headers for each timestamp we want to resolve. I think this approach is fine for now, and in this context, considering that this RPC is a slow and heavy one anyways. Worst case is that we need to fetch ~20 block headers per timestamp we look up.
Future improvements
If we want to optimize the timestamp to block height resolving more, we could make use of this public API endpoint: https://mempool.space/docs/api/rest#get-block-timestamp
This endpoint seems to do a lookup from an internal DB (see here), which is likely more performant. In that case the current binary search mechanism could serve as only a fallback for if the mempool.space API is unavailable
We can also make use of pagination when querying onchain activity from the LND backend. This seems to require changes in the lndclient repository as well, to ensure that the
GetTransactionsRPC can be called with aindex_offsetandmax_transactions, like described herePull Request Checklist
MinLndVersionif your PR uses new RPC methods or fields oflnd.