Add Fee Estimation Documentation & Best Practices #270
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for lightningdevkit ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
ConorOkus
reviewed
Jul 19, 2024
| } | ||
| ``` | ||
|
|
||
| ## The ```ConfirmationTarget``` |
There was a problem hiding this comment.
I think a link to the rust API docs is sufficient here so we could remove the table on this page. That way we can avoid duplication and having to update in two places.
There was a problem hiding this comment.
Noted! I removed the table and simply provided a link to the Rust API Docs instead.
There was a problem hiding this comment.
Might be worth going through and cleaning up those docs at some point, FWIW.
ConorOkus
reviewed
Jul 19, 2024
|
|
||
| Running your own Bitcoin node can relieve some of these concerns, but it may require additional technical expertise and maintenance to operate properly. These trade-offs should be considered carefully when deciding on how your application will retrieve fee estimates. | ||
|
|
||
| A savvy reader may have noticed that the above architecture is not very performant. LDK may generate a substantial number of fee-estimation calls in some cases. So, if we have to call an API every time we want a fee estimate, we'll add a significant amount of additional load to our application, and we may substantially slow HTLC handling. To ensure your application remains performant, you should pre-calculate and cache the fee estimates so that they can simply be retrieved when needed. Additionally, it's recommended to refresh fee estimates every 5-10 minutes so that your application has access to fresh freerates. |
There was a problem hiding this comment.
We might also note that many of of these third parties are rate-limited so devs should handle that case to.
There was a problem hiding this comment.
There is some commentary about rate-limiting just above this section. Specifically, it says:
- Third parties often rate-limit users such that they can only access a limited number of fee estimates within a given timeframe. If you require a certain service level agreement (SLA), it's recommended to run your own Bitcoin node or explore paid services with SLA guarantees, such as mempool.space enterprise accounts.
Let me know if you think we should add more.
ConorOkus
reviewed
Jul 19, 2024
docs/fee_estimation.md
Outdated
| ``` | ||
|
|
||
| ### Building A Fall-Back Plan | ||
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mepool.space. |
There was a problem hiding this comment.
Suggested change
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mepool.space. | |
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mempool.space. |
Suggested change
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mepool.space. | |
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mepool.space. |
Suggested change
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mepool.space. | |
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mepool.space. |
Suggested change
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mepool.space. | |
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mempool.space. |
Suggested change
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mepool.space. | |
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mempool.space. |
There was a problem hiding this comment.
Good catch. Fixed!
ConorOkus
approved these changes
Jul 19, 2024
|
@jkczyz - I see you added yourself as a reviewer. Let me know if you have any feedback. One question I'm still pondering is if this documentation should be a single page (ex: Key Management) or if it should be collapsable (ex: Blockchain Data section). If it's collapsable, it would have the following sub-sections:
If you or @ConorOkus have an opinion, let me know. If not, I'll probably move forward with making it collapsable and adjusting this PR to include the associated code changes to the LDK website. |
|
Sorry about the delay. Will take a look today. Regarding the layout, I'm somewhat indifferent. Might even consider making the "Blockchain Data" section one page. I don't mind a long page as long everything is well-delineated and doesn't come across as too intimidating (e.g., large blocks of text). And if cross referencing other sections is useful, one page may be better suited. |
jkczyz
reviewed
Aug 6, 2024
| @@ -0,0 +1,419 @@ | |||
| # Fee Estimation | |||
There was a problem hiding this comment.
Could you update https://github.com/lightningdevkit/lightningdevkit.org/blob/main/docs/.vuepress/config.js accordingly so that the page shows up in the preview?
docs/fee_estimation.md
Outdated
| # Fee Estimation | ||
|
|
||
| ## Overview | ||
| LDK provides a ```FeeEstimator``` trait which, once implemented, assists in variety of on-chain and off-chain functions in the LDK stack. It's important to note that the ```FeeEstimator``` only describes the architectural requirements that must be satisfied to successfully incorporate LDK into your project. It does not, itself, provide the logic or implementation to calculate a feerate. |
There was a problem hiding this comment.
Drop "in the LDK stack", IMO.
docs/fee_estimation.md
Outdated
| Feerates and their functionality within LDK can be conceptually separated into on-chain (Bitcoin) and off-chain (Lightning Network) tasks. | ||
|
|
||
| #### On-Chain | ||
| Lightning's security model is dependent on the ability to confirm a claim transaction under a variety of circumstances, such as before a timelock expires. To ensure that LDK is able to confirm claim transactions in a timely manner or bump the fee on an existing transaction, LDK utilizes the ```FeeEstimator``` to fetch feerates for high priority transactions. For example, the ```OnChainSweep``` ```ConfirmationTarget``` specifies the feerate used when we have funds available on-chain that must be spent before a certain expiry time, beyond which our counterparty could potentially steal them. Additionally, the ```OutputSpendingFee``` ```ConfirmationTarget``` is used by the ```OutputSweeper``` utility to ensure that, after channel closure, all funds are eventually swept to an onchain address controlled by the user. |
There was a problem hiding this comment.
Let's use the rust notation for variants: Enum::Variant (e.g., ConfirmationTarget::OnChainSweep).
docs/fee_estimation.md
Outdated
| Lightning's security model is dependent on the ability to confirm a claim transaction under a variety of circumstances, such as before a timelock expires. To ensure that LDK is able to confirm claim transactions in a timely manner or bump the fee on an existing transaction, LDK utilizes the ```FeeEstimator``` to fetch feerates for high priority transactions. For example, the ```OnChainSweep``` ```ConfirmationTarget``` specifies the feerate used when we have funds available on-chain that must be spent before a certain expiry time, beyond which our counterparty could potentially steal them. Additionally, the ```OutputSpendingFee``` ```ConfirmationTarget``` is used by the ```OutputSweeper``` utility to ensure that, after channel closure, all funds are eventually swept to an onchain address controlled by the user. | ||
|
|
||
| #### Off-Chain | ||
| From an off-chain perspective, the ```FeeEstimator``` provides vital information that is used when opening, closing, and operating channels with peers on the Lightning Network. For example, when opening an inbound channel or updating an existing channel with a counterparty, LDK will compare the counterparty's proposed feerate with the minimum feerate that is allowed for that type of channel (ex: anchor or non-anchor channel). If the proposed fee is too low, LDK may return and error and suggest to close the channel. During these operations, LDK will reference the ```MinAllowedAnchorChannelRemoteFee``` ```ConfirmationTarget``` and the ```MinAllowedNonAnchorChannelRemoteFee``` ```ConfirmationTarget```. Therefore, it's recommended to ensure that the minimum allowable fees are not set too high, thus increasing the risk that a peer's proposed feerate is too low, potentially resulting in a force closure. See the **Best Practices** section in this documentation for more helpful tips when implementing LDK's ```FeeEstimator```. |
There was a problem hiding this comment.
Could you add a link for "Best Practices"?
docs/fee_estimation.md
Outdated
| ``` | ||
|
|
||
| ## The ```ConfirmationTarget``` | ||
| The ```ConfirmationTarget``` enables the user to define the feerate that LDK will use for a variety of on-chain and off-chain tasks. For a detailed description of each ```ConfirmationTarget```, please visit [here](https://docs.rs/lightning/latest/lightning/chain/chaininterface/enum.ConfirmationTarget.html#variant.OnChainSweep). |
There was a problem hiding this comment.
Could you link to the docs.rs instead? https://docs.rs/lightning/latest/lightning/chain/chaininterface/enum.ConfirmationTarget.html
docs/fee_estimation.md
Outdated
| ``` | ||
|
|
||
| ### Building A Fall-Back Plan | ||
| Great, we now have a function that will return a feerate, provided by mempool.space, for a given ```ConfirmationTarget```. But, there is a vulnerability here! What if mempool.space is down or our API call fails for some reason? If we're in urgent need of a feerate estimate, we'll need to return *something*. In a real production environment, we'd likely have a local mempool that we're sourcing feerates from and, perhaps, one ore more third-party services. However, for this demo, we'll hard-code fallback feerates and return those if we're unable to retrieve a feerate from mempool.space. |
docs/fee_estimation.md
Outdated
Comment on lines
231
to
261
| pub async fn new() -> Result<Self, reqwest::Error> { | ||
| let mut fee_rate_cache = HashMap::new(); | ||
|
|
||
| // Attempt to fetch fee rates from the mempool | ||
| let fee_rates = Self::fetch_fee_from_mempool().await; | ||
|
|
||
| match fee_rates { | ||
| Ok(rates) => { | ||
| // Store the fetched rates in the cache | ||
| fee_rate_cache.insert(ConfirmationTarget::OnChainSweep, rates.fastest_fee * 250 as u32); | ||
| fee_rate_cache.insert(ConfirmationTarget::OutputSpendingFee, rates.fastest_fee * 250 as u32); | ||
| fee_rate_cache.insert(ConfirmationTarget::NonAnchorChannelFee, rates.economy_fee * 250 as u32); | ||
| fee_rate_cache.insert(ConfirmationTarget::AnchorChannelFee, rates.minimum_fee * 250 as u32); | ||
| fee_rate_cache.insert(ConfirmationTarget::ChannelCloseMinimum, rates.minimum_fee * 250 as u32); | ||
| fee_rate_cache.insert(ConfirmationTarget::MinAllowedNonAnchorChannelRemoteFee, rates.minimum_fee * 250 as u32); | ||
| fee_rate_cache.insert(ConfirmationTarget::MinAllowedAnchorChannelRemoteFee, rates.minimum_fee * 250 as u32); | ||
| }, | ||
| Err(_) => { | ||
| // Use fallback fees if the API call fails | ||
| use ConfirmationTarget::*; | ||
| for target in &[MinAllowedAnchorChannelRemoteFee, MinAllowedNonAnchorChannelRemoteFee, ChannelCloseMinimum, AnchorChannelFee, NonAnchorChannelFee, OnChainSweep, OutputSpendingFee] { | ||
| let fallback_fee = Self::fallback_fee_from_conf_target(*target); | ||
| fee_rate_cache.insert(*target, fallback_fee); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(MyAppFeeEstimator { | ||
| fee_rate_cache | ||
| }) | ||
| } |
There was a problem hiding this comment.
Alternatively, we may want to initialize the default fees in a non-async constructor. Then have an async function for updating the fees.
| ``` | ||
|
|
||
| ### Demo Codebase | ||
| If you've been following the demo or would like to run/experiment with this code yourself, the entire main.rs file is provided below. |
There was a problem hiding this comment.
Note that we may want to link to a repo with this code.
docs/fee_estimation.md
Outdated
|
|
||
| In the below code, we start by creating ```fee_estimator```, an instance the ```MyAppFeeEstimator```. We then define two fee targets, ```high_fee_target``` and ```low_fee_target``` for the ```OnChainSweep``` and ```MinAllowedAnchorChannelRemoteFee``` ```ConfirmationTarget```. Finally, we call pass these confirmation targets into ```fee_estimator.get_est_sat_per_1000_weight``` and print the feerates (sats/KW) to the terminal. | ||
|
|
||
| One best-practice that was not implemented in this demo but is left as an exercise to the developer is refreshing the updates every 5-10 minutes. One approach toward accomplishing that would be to keep track of the last time the ```fee_estimator``` fees were refreshed and create a background process to update those fees periodically. Another exercise that is left to the reader is to create additional functionality within the ```MyAppFeeEstimator``` to retrieve the feerate for custom circumstances. For example, you could create a function called ```get_high_feerate```, which takes no arguments and, instead, returns the feerate that should be used for a high-priority transaction. |
There was a problem hiding this comment.
Additionally, we could link to LDK Node's fee estimator.
https://github.com/lightningdevkit/ldk-node/blob/main/src/fee_estimator.rs
| } | ||
| ``` | ||
|
|
||
| ## The ```ConfirmationTarget``` |
There was a problem hiding this comment.
Might be worth going through and cleaning up those docs at some point, FWIW.
|
Thanks for the detailed comments, @jkczyz! I've updated the docs accordingly. The only feedback I haven't incorporated yet is creating a GitHub repo with an example code base for the reader to clone/fork. If we think that's important to include before merging, I can add it in. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a first-draft submission for
FeeEstimatordocumentation (including best practices). Once complete, it will close #153.As of now, I submitted the documentation as a single file, since this will make it easier for others to read and provide comments. However, once the final edits are made, I think it would look best if I made it collapsable - like how the Blockchain Data section on the LDK Docs website has multiple sub-sections (ex: Introduction, Chain Activity, etc.). If this documentation followed that model, it would have the following sub-sections:
Once we finalize the edits and align on the right format, I will update the PR to include the necessary changes to the lightningdevkit.org website so that the fee estimation documentation will be available.
Looking forward to receiving feedback!