This is very much a WIP, but feedback is welcome.
This adds support for using the mempool state when calculating the fee rate for transactions. It is currently only supported in estimatesmartfee RPC command, but intention is to make it available in sendtoaddress and friends eventually.
To do (at minimum):
(0.1+(conservative*0.05))*max(0.5, 1-0.1*confTarget))807 | @@ -805,10 +808,11 @@ UniValue estimatesmartfee(const JSONRPCRequest& request) 808 | + HelpExampleCli("estimatesmartfee", "6") 809 | ); 810 | 811 | - RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR}); 812 | + RPCTypeCheck(request.params, {UniValue::VNUM, UniValue::VSTR, UniValue::VBOOL});
I think it would be helpful if you could post (or link to a gist) the overall design you're aiming for here, so that reviewers can think about the algorithm's robustness without having to dive into the implementation first.
I did start to take a look, and I wanted to point out that the first commit looks incorrect -- GetSortedDepthAndScore() does not return the mempool sorted in the same way as block creation. Instead it sorts the mempool by ancestor count first (so all transactions with no ancestors appear before their descendants), and by feerate of the transaction second (not feerate including ancestors). The mining sort order is based on sorting transactions by their feerate with ancestors (which is done in one of the mempool.mapTx indices), and then updating that feerate-with-ancestors calculation as transactions are selected for inclusion.
So a simple approximation of the mining calculation would to walk the mempool in the ancestor-feerate-order, and tally up the weight of transactions as you go. In order to get it precise, though, you'd probably just want to invoke CreateNewBlock() directly.
I think it would be helpful if you could post (or link to a gist) the overall design you're aiming for here, so that reviewers can think about the algorithm's robustness without having to dive into the implementation first.
Sorry, I should have just finished reading the PR -- I think I understand it now, never mind!
@sdaftuar Thanks for looking!
the first commit looks incorrect --
GetSortedDepthAndScore()does not return the mempool sorted in the same way as block creation. Instead it sorts the mempool by ancestor count first (so all transactions with no ancestors appear before their descendants), and by feerate of the transaction second (not feerate including ancestors).
I wanted to ask about that. Thanks for clarifying -- I would like to avoid the block creation code, as I think it results in side effects that are not desirable (I believe early prototypes of testrawtransaction had issues due to trying to reuse that functionality). I could be mistaken. Anyway, that's a pity. I was hoping there was a clean way to get the transactions in order. I also found out that the ancestor stuff actually plays a big role in blocks, so I may need to take that into account after all.
After making this PR, I realized I was a bit hasty. I need to do proper profiling before making a serious proposal. I'm considering closing, but I think it's fine since it has [WIP] tag...?
CreateNewBlock should be side effect free (the issue you're bringing up with testrawtransaction and the like involves mempool acceptance testing, which is not side-effect free). It's not super-fast (especially because we test block validity after construction), but we could consider just exposing the package selection algorithm if that's helpful here.
I don't see a problem with keeping this open with [WIP] in order to gather more feedback. I do think it's worth trying to have a wider discussion about how to best expose data relating to fee estimates, to let advanced users have access to useful data which may still be experimental while ensuring that default usage for typical users is robust.
I'm closing this but will reopen when I'm closer to an actual PR.
