chainfee: allow specifying min relay feerate from the API source #8891
Conversation
yyforyongyu
added
utxo sweeping
itests
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
yyforyongyu
force-pushed
the
fix-fee-estimator
branch
from
6c41661
to
dd4743b
Compare
| @@ -816,22 +816,24 @@ func (w *WebAPIEstimator) EstimateFeePerKW(numBlocks uint32) ( | |||
| // | |||
| // NOTE: This method is part of the Estimator interface. | |||
| func (w *WebAPIEstimator) Start() error { | |||
| log.Infof("Starting Web API fee estimator...") | |||
There was a problem hiding this comment.
probs just my friday morning brain not kicking in yet but is this behaviour actually different? cause Start wont return before w.started.Do is run right? so does it matter if updateFeeEstimates() is called here or in started.Do?
scenario 1: noCache = true:
- we return early here but that's ok cause
EstimateFeePerKWwill then always callw.updateFeeEstimates()before looking at the cache.
scenario 2: noCache = false:
started.Doruns and callsw.updateFeeEstimatebefore returning fromStart.EstimateFeePerKWonly ever called afterStartis done soupdateFeeEstimatewould have been called
am I missing something here? (i probs just need more coffee)
There was a problem hiding this comment.
yeah you are totally right! For some reason, I thought it was waiting for the ticker to fire before doing the first updateFeeEstimate. Will remove this commit. Meanwhile I realized that if Start is still finishing up, yet another call to EstimateFeePerKW is made, we could still get a floor feerate. This should never happen as we always wait for the Start to be finished first, but just in case.
There was a problem hiding this comment.
This can be avoided by inlining the Start method into the constructor.
yyforyongyu
force-pushed
the
fix-fee-estimator
branch
from
dd4743b
to
e1fe2ed
Compare
yyforyongyu
force-pushed
the
fix-fee-estimator
branch
from
e1fe2ed
to
9871b3e
Compare
| if resp.MinRelayFeerate == 0 { | ||
| log.Errorf("No min relay fee rate available, using default %v", | ||
| FeePerKwFloor) |
| // GetFeeInfo will query the web API, parse the response and return a map of | ||
| // confirmation targets to sat/kw fees and min relay feerate in a parsed | ||
| // response. | ||
| func (s SparseConfFeeSource) GetFeeInfo() (WebAPIResponse, error) { |
There was a problem hiding this comment.
could return a pointer if you want. It was a pointer before & then dont need to return empty instances
There was a problem hiding this comment.
We should avoid pointer usage when constructing something from scratch.
There was a problem hiding this comment.
why? we use that pattern everywhere & is a pretty standard golang pattern afaiaa
There was a problem hiding this comment.
- Why open yourself up to reference aliasing if the thing is being constructed for the first time right there. It obscures the ownership semantics of the data itself. This causes issues where the code becomes more brittle and subject to segmentation faults. If you are constructing something for the first time, you -- by definition -- own it. Owned data should not be behind pointers. We use pointers to lease data to other functions that either read from it or do scoped modification.
- Why do the heap allocation when you can do it on the stack? Heap allocation is significantly slower. Additionally pointer dereferences themselves are not free either.
There was a problem hiding this comment.
FWIW go does recommend pass by values.
| testFees := map[uint32]uint32{ | ||
| 1: 12345, | ||
| } | ||
| testResp := struct { |
| @@ -792,6 +793,12 @@ func NewWebAPIEstimator(api WebAPIFeeSource, noCache bool, | |||
| func (w *WebAPIEstimator) EstimateFeePerKW(numBlocks uint32) ( | |||
| SatPerKWeight, error) { | |||
|
|
|||
| // If the estimator hasn't been started yet, we'll return an error as | |||
| // we can't provide a fee estimate. | |||
| if !w.started.Load() { | |||
There was a problem hiding this comment.
cool - I guuuuuueeeess there is no harm adding this but in general I like the assumption that Start is called & completed before anything else is called & so is by definition thread safe & deterministic. Cause the other side of the spectrum is doing this isStarted check on every API method which is not ideal I think
There was a problem hiding this comment.
Or we could move away from start methods entirely and just start it in the constructor when it is called. IMO we should only have Start methods for things that primarily interact through channels.
This is known as RAII wherein the primary assumption is that a resource is ready to use when it is constructed.
There was a problem hiding this comment.
RAII can be nice when using OOP. For instance in python you can define what happens when initializing the object in its __ini__. Here we may achieve something similar using the NewXXX method, which is better than embedding the Start method in the constructor (IIUR about this term) as, there might be multiple constructors, we need to carry this concern in every one of them, just like @ellemouton pointed out that the started now has to be checked on every API.
Also this pattern has already been used in some of the RPC calls, so we make sure the server is fully started before responding to requests. This led me to think about what NewXXX and Start should accomplish, maybe we should start inside the NewXXX so we can be somewhat certain that a given state is reached once the struct is returned. However that's a bigger refactor so I kinda let it go, maybe worth revisiting when it becomes an issue someday (for instance #8166)
There was a problem hiding this comment.
IMO the Start/Stop APIs only make sense if we can create a state machine that is capable of multi-start/multi-stop. However, we have (for some reason) intentionally avoided this. Absent multi-start/multi-stop, the lifetime of the object should dictate the lifetime of the goroutines it launches. Therefore one of the things we can do here is launch the goroutines in whatever the NewXXX method is, and then hook into the GC to call XXX.stop() when all of the XXX refs go out of scope using a gc finalizer
Definitely agree that this is not the place for that much larger discussion to unfold though. Just wanted to share my thoughts as I have found that the Start/Stop pattern in the codebase to be a wart that doesn't make sense, and people seem to just follow the pattern because "that's what we've always done".
yyforyongyu
force-pushed
the
fix-fee-estimator
branch
from
9871b3e
to
21d72fb
Compare
This commit adds a new expected field, `min_relay_feerate`, in the response body returned from the API source, allowing the API to specify a min relay feerate to be used instead of the FeePerKwFloor. This change is backwards compatible as for an old API source which doesn't specify the `min_relay_feerate`, it will be interpreted as zero.
Previously we may get a floor feerate when calling `EstimateFeePerKW` due to the fee estimator not finishing its startup process, which gives us an empty fee map. This is now fixed to return an error if the estimator is not started.
yyforyongyu
force-pushed
the
fix-fee-estimator
branch
from
21d72fb
to
e772e70
Compare
lnutils/log.go
Outdated
| package lnutils | ||
|
|
||
| // LogClosure is used to provide a closure over expensive logging operations so | ||
| // don't have to be performed when the logging level doesn't warrant it. | ||
| type LogClosure func() string | ||
|
|
||
| // String invokes the underlying function and returns the result. | ||
| func (c LogClosure) String() string { | ||
| return c() | ||
| } | ||
|
|
||
| // NewLogClosure returns a new closure over a function that returns a string | ||
| // which itself provides a Stringer interface so that it can be used with the | ||
| // logging system. | ||
| func NewLogClosure(c func() string) LogClosure { | ||
| return LogClosure(c) | ||
| } |
lnwallet/chainfee/log.go
Outdated
| // logClosure is used to provide a closure over expensive logging operations so | ||
| // don't have to be performed when the logging level doesn't warrant it. | ||
| type logClosure func() string | ||
|
|
||
| // String invokes the underlying function and returns the result. | ||
| func (c logClosure) String() string { | ||
| return c() | ||
| } | ||
|
|
||
| // newLogClosure returns a new closure over a function that returns a string | ||
| // which itself provides a Stringer interface so that it can be used with the | ||
| // logging system. | ||
| func newLogClosure(c func() string) logClosure { | ||
| return logClosure(c) | ||
| } |
lnwallet/channel.go
Outdated
| @@ -1869,7 +1870,7 @@ func (lc *LightningChannel) restoreCommitState( | |||
| lc.localCommitChain.addCommitment(localCommit) | |||
|
|
|||
| lc.log.Tracef("starting local commitment: %v", | |||
| newLogClosure(func() string { | |||
| lnutils.NewLogClosure(func() string { | |||
There was a problem hiding this comment.
I'm thinking it could be good to add to the lnutils package a function specifically designed to handle the spew case
func SpewLogClosure(a interface{}) func() string {
return func() string {
return spew.Sdump(a)
}
}
And replaces all usage of `logClosure` with `lnutils.LogClosure`.
yyforyongyu
force-pushed
the
fix-fee-estimator
branch
from
e772e70
to
d992cf9
Compare
In an attempt to test the sweeper's
Aggregator's filtering logic, where it would filter out the input whose budget cannot cover the min relay fee, I realized there's no way to set the min relay feerate in our test, which led to this PR.This PR adds a new field to the expected resp returned from the API, allowing the service to specify a min relay feerate to be used. A bug is also found, which could result in a floor feerate being used when calling
EstimateFeePerKWas there's no cached created during the startup, yet the ticker hasn't fired.Partially address #8688.