Fee Estimation Functionality #1486
Conversation
|
😍 |
|
Brilliant will try to review once I have time. |
|
This seems to be humongous PR, putting critical label on it because of that. Quick question – is this just support of bitcoin core code or is it something else? After quickly looking into the code, it seems that it is very much bitcoin oriented and I'm not sure if or how much it is compatible with Stratis. Can we have any information about that please? |
|
It's just port of bitcoin new algorithm of fee estimation calculation. Changes were made mainly in BlockPolicyEstimator and TxConfirmStats classes. Also FeeTest was changed according to new algorithm (ported from bitcoin also). Nothing more. Calling of this algorithm from other parts of a system will be added today. |
|
In that case I think we will need some changes to make it more generic. I'm unable to say how big those changes will have to be. However I can see many constants that looks like very much bitcoin only. This could be a problem as we are working on solutions that must support different coins and must not be tightly coupled with the specific configuration of single coin. Therefore I think we would need to identify those points in this code and make them more generic. |
|
Understood. I'll think about it and present some suggestions. Task description did not contain these details, so I just applied bitcoin algorithm as is. |
|
I am not sure that are that much differences with bitcoin when it comes to fee estimation. |
|
After more detailed review of algorithm it seems to me that all constants, which stated it estimator, are connected rather to algorithm itself, not to specifics of blockchain. So I can move these constants to separate class, which can be accessed from NodeSettings. I guess, this class will be the same for bitcoin and stratis network. The only question is how to fill these parameters. From file? |
|
According to smart contracts - as far as I understood, we shouldn't add sc fee to data for fee estimation (buckets). How can we distinguish them from casual txs on mempool level? |
| ///<summary>Historical estimates that are older than this aren't valid</summary> | ||
| private const int OldestEstimateHistory = 6 * 1008; | ||
|
|
||
| ///<summary>Decay of .962 is a half-life of 18 blocks or about 3 hours</summary> |
There was a problem hiding this comment.
All these comments suggest that this is bitcoin specific, at least the comments themselves are bitcoin specific it it is a question whether the same number of blocks should be used for other block chains.
There was a problem hiding this comment.
Yes, I see, I thought that horizons are rather counted in blocks, not in time intervals, so, e.g. medium horizon should be 48 blocks, no matter what is block time in concrete blockchain. I can suggest to make pair of settings for every horizon - for blocks and for minutes (or hours). E.g. MedBlocks and MedTime. If MedBlocks is filled - algorithm just gets its amount as block amount. If MedTime filled - algorithm gets time and divides it by block time for certain blockchain. It gives some flexibility in algorithm setup. Because honestly, I don't know what will be more correct - keep horizon duration the same as bitcoin has in blocks or in amount of time.
There was a problem hiding this comment.
About horizon decays - I can make them flexible by calculating them from block time (e.g. for bitcoin "Short history: 0.962 decay (half-life 18 blocks, 3 hours)", then for another blockchain it should be another number to suit this 3 hours)
There was a problem hiding this comment.
I'm not familiar with the algorithm yet, I just wanted to show you one example of what seems to be specific for bitcoin. You shouldn't then ask yourself if the same number should be used for blockchain which has different block size or different target spacing or different maximum number of transaction in a block or different recommended number of confirmations to wait for or different whatever.
If you realise that you really want to use the same number, then you just need to modify the comment appropriately so that it is not bitcoin specific. Otherwise you're looking for some kind of formula that will get you the correct number on each blockchain based on its parameters.
|
Don't worry about smart contracts. We can work around whatever happens here. We will have to sort transactions in the mempool separately, and we do add funds to the fee to account for gas usage, so modularity wherever possible is handy however. |
|
@AndrewZvvv I will review this tomorrow, please merge latest changes. We will probably have to try to make this interface based and injectable to the mempool, then when we use bitcoin we can inject otherwise perhaps use a default empty estimators for stratis. |
|
@dangershony Committed my last changes, but there are still no changes about:
In this commit there are:
|
|
This one is quite huge. Is anyone going to review it? @dangershony @bokobza If not I'll try to dive into it, let me know. |
|
This is a huge code change indeed but its isolated to fee policy and show not change much apart for ht existing fee policy code that did not work anyway (and was not activated) I will look in to this once I have time but feel free to dig in as well @noescape00 |
|
As this is huge, let's try to identify separated tasks for reviewers. The following is just the first attempt, feel free to identify more separated task as you become familiar with the code. At any point, anyone can just take a task from this list, do it, and the report that it has been done. You can also take a task and try to identify separated subtasks. Task one – code style – go through whole PR and strictly enforce code style. Task two – comments – go through whole PR and read all comments, mention where it is needed to improve the grammar, style, content, et cetera. Task three – logs – go through whole PR and make sure good logs are implemented according to our style. Task four – good separation of protocol specific constants – go through the whole PR and think about all the constants that are used in the algorithm. If you see anything that is specific to the bitcoin, make sure it is properly separated and that for Stratis network a proper value is used instead. Task five – compare to bitcoin core – as this is supposed to be a port of the logic and algorithm from bitcoin core, we should compare the C# code with the bitcoin core code as the developer might have make a mistake while porting. We want to make sure the logic is equivalent to the original. Task six – understand the logic – try to understand the logic of this algorithm. Then try to question whether it fits only bitcoin or if it is usable for arbitrary network that we want to support, focus on Stratis of course. If any changes are needed in order to support other networks, mention that. Task seven – tests – Check all tests, check whether the coverage is sufficient, if the tests are ported from bitcoin check whether they match. |
src/Stratis.Bitcoin.Features.MemoryPool/Fee/SerializationEntity/BlockPolicyData.cs
Show resolved
Hide resolved
src/Stratis.Bitcoin.Features.MemoryPool/Fee/SerializationEntity/TxConfirmData.cs
Show resolved
Hide resolved
| /// <summary>Map of bucket upper-bound to index into all vectors by bucket.</summary> | ||
| private SortedDictionary<double, int> bucketMap; | ||
|
|
||
| private object lockObject; |
There was a problem hiding this comment.
What is the contract of this object? What is it supposed to protect?
|
Quite confused here, what is Algorithm014? It is the old one or the new one? And which one is used? Also, how does the used one work with reorganisations? Is everything recalculated when reorganisation happens? |
|
Definitely a lot of work is needed to make it is to comply with current code style and logging style policies as well as the commenting syntax and scope. |
|
@AndrewZvvv tests are failing, action is needed on @Aprogiena 's review, there are merge conflicts |
|
Unfortunatelly, I have no free time now, I will try to fix issues in a week or two. Thank you for your time and for review. |
|
This PR is greatly outdated so far. @AndrewZvvv are you going to work on it? Also if anyone would like to pick it up- please write here. Otherwise I think it makes sense to close the PR |
|
We will push it at some point we just need to get other "things" out of the way so happy to keep it here (if we close it we should create an issue thats tracking the close PR and reopen it at some point) |
|
Created an issue for this: #2295 |
|
Continuing this in #4040 |
Fee estimation algorithm rewrited according to new bitcoin algorithm, added read/write estimation state to a file, FeeTest changed according to new algorithm