Switch to weight units for all feerates computation #17566
Conversation
darosior
force-pushed
the
feerate_in_weight
branch
from
d6bb55d to
40063b8
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
darosior
force-pushed
the
feerate_in_weight
branch
3 times, most recently
from
faa72fe to
2542ca1
Compare
|
Rebased now that appveyor seems to be fixed. |
darosior
force-pushed
the
feerate_in_weight
branch
2 times, most recently
from
720da2e to
dfa7655
Compare
|
This might be "too late" but the fee estimation can be floored to 253 sat/kWU, so that the wallet doesn't create transaction below old feerate until most of the network relay 250 sat/kWU txs.. |
darosior
force-pushed
the
feerate_in_weight
branch
from
dfa7655 to
ee24407
Compare
darosior
force-pushed
the
feerate_in_weight
branch
2 times, most recently
from
732a5a7 to
5817a3e
Compare
|
Rebased. |
|
I can't help but wonder if there's a better abstraction here instead of random conversions happening everywhere... |
Conversion is needed anyway for every user input (and even for JSONRPC outputs) to not break the API.. |
|
> I can't help but wonder if there's a better abstraction here instead of random conversions happening everywhere...
Conversion is needed anyway for every user input (and even for JSONRPC outputs) to not break the API..
yes I understand that conversions are needed... I'm just referring to
the fact these are two different fee types, but that fact is not
captured in the type system. If we had a better abstraction that
captures both of these types, it would be less error prone and we
wouldn't have to opencode conversions everywhere.
|
Since Segregated Witness activation and the transaction size being determined in wieght units, our internal logic for transaction size (and thus feerates) has relied on virtual bytes. Virtual bytes are useful for non-technical end-user experience, but can cause some issues if the whole logic rely on this raw division. For instance, if stored as an integer, the transaction size will be truncated, which can lead to wrong feerate calculation. In order to patch this, GetVirtualTransactionSize() rounds up. This led to not only protecting the users from paying too-low fees and seeing their transaction rejected, but this also implicitly increased the minimum feerate for relaying transactions. For instance a minrelayfee(rate) of 1000sat/kvbyte is of 253sat/kWU, while we could expect it to be 250sat/perkWU (see issue # 13283). Using constants in weight units will allow us to avoid raw roundings for feerate computation and to get an accurate feerate estimation (and a deterministic relay policy), while keeping the cosiness of vbytes for non-technical end-users.
This adds transaction size utils in weight where there were only virtual byte ones. This will allow to use weight units for feerate computation later on while keeping accurate vbyte computation for not breaking the API.
darosior
force-pushed
the
feerate_in_weight
branch
from
5817a3e to
74c3c73
Compare
|
Rebased. Will rework the approach and include remarks from jb55 if there are some acks on the general concept of using / exposing weight units.. |
Most constants are unchanged but timed by 4 at initialization, mainly to avoid a big diff by rewriting all 'GetArg's because we need to still expose parameters as taking vbytes.
This adds weight units counter to the 'CTxMemPoolModifiedEntry' struct to make weights the packages size unit. This also treats block min feerate as per weight units, while leaving intact the vbyte shroud for the user.
The functional tests in wallet_basic.py are also modified to check feerates using weight units, as we don't round up anymore for transaction size computation.
darosior
force-pushed
the
feerate_in_weight
branch
from
74c3c73 to
3dc4f5d
Compare
| Needs rebase |
|
Concept ACK. Correctness of fee computation is safety-critical for any time-sensitive protocol like LN. Overpaying by default isn't the solution as you may have to save value for period of congestion. With this regards easing implementation of segwit fee-estimators should be highly considered. As Rusty points out in related issue, this should be part of a wider discussion on a stable API for protocol applications on top of Bitcoin Core. |
|
Thank you for the feedback, I'll rebase this and change the approach as per jb55's comments soon.
I wanted to go for the minimal changes, but something cleaner is definitely better. This makes me wonder
if I should break this into smaller PRs, too.
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐
Le lundi, juin 22, 2020 11:31 AM, Antoine Riard <[email protected]> a écrit :
… Concept ACK.
Correctness of fee computation is safety-critical for any time-sensitive protocol like LN. Overpaying by default isn't the solution as you may have to save value for period of congestion. With this regards easing implementation of segwit fee-estimators should be highly considered.
As Rusty points out in related issue, this should be part of a wider discussion on a stable API for protocol applications on top of Bitcoin Core.
—
You are receiving this because you authored the thread.
Reply to this email directly, [view it on GitHub](#17566 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F3A3CXEMCDW6WC6QPDRX4QIJANCNFSM4JQWGURQ).
|
|
I gathered some opinions on this from a long time contributor about the concept, he replied: After looking at the code, however, his reaction was that the patch was too big for the benefit. Putting more data in the CTxMemPoolEntry especially stood out as an issue. @darosior I think it's worth getting more conceptual review before you put in more effort. I believe it's clear that more discussions need to happen around ways to improve the linkages between Bitcoin Core and LN, but based on the opinions I gathered, the approach on this PR could use some more feedback. |
|
@adamjonas this PR is a bad approach. It's been known and discussed above, so I don't think it's worth discussing it any more. However for the sake of the argument: I locally have a version which is an about 3x smaller diff, and regarding adding a field to the CTxMempoolEntry it's necessary anyway as to use weight-mesured feerates we need to track, well, the weight of the tx. I agree it needs more conceptual feedback, but i don't want to ask for it before this patchset is reworked (will push soon ™️ ) as i think the boutique approach here could degrade the conceptual review. Nonetheless, thank you for your interest. |
|
This has required rebase since February, and there are still conceptual questions outstanding. I'm going to close it for now. @darosior - feel free to reopen (or open another PR) if you have something that's ready for review/discussion. |
7 participants
This changes the current feerate computation from using virtual bytes to weight units for transaction size.
The transaction size being stored and used as virtual bytes introduced some non-negligible rounding and approximations (See #13283 for an example.). This patch set is an attempt to switch to using weight units in place of virtual bytes internally for a more accurate feerate computation, while leaving intact the vbyte interface.
There is, I think, a big cleanup needed (for example renaming feerate constants as
xx_FEERATEwould really improve readability) but I wanted to keep changes as minimal as possible.Fixes #13283.