No more base weight #21
Conversation
- CoinSelector no longer tracks anything but input weight - Previously the value of the target outputs was in `Target` but the weights were accounted for in CoinSelector. Now they're in all in target. - This allows us to actually figure out how many outputs there are and therefore the actual weight of the transaction accounting for varints.
LLFourn
force-pushed
the
no_more_base_weight
branch
from
5482f81 to
7e82c3f
Compare
changeless should not be that slow but if it fails to find a changeless solution then the test will exhaustive search so that clearly will take forever.
|
Yeah, the waste metric is only used as a tiebreaker rather than something to optimize for because it would happily consolidate all your UTXOs at low feerates. What did you end up using instead? Sorry, I only discovered this codebase recently per the Optech Newsletter, I’ll take a bit of a look around. :) |
|
Hey @murchandamus! We've been keeping this code base low profile because it's still in science experiment stage. FWIW we gathered a long time ago that the waste metric was not something that you should use to actually optimize for -- it was just a good one to use to start the experiment. I'm interested in your work here: https://delvingbitcoin.org/t/gutterguard-and-coingrinder-simulation-results/279. It would be cool to try and replicate it. We're just using what we call |
|
Ah that sounds similar to what I’m trying to do with CoinGrinder, where I am trying to always find the input set with the lowest input weight. The PR should hopefully be very readable: I organized the pullrequest to start with a simple implementation of the algorithm and then each improvement or optimization a separate commit. I still have a little hope that it might make the release, even if I did miss the really big feerates. In combination with BnB, and the waste metric, I guess I’d get something similar as the outcome of your |
We already have const `Drain::NONE` which should be used instead.
There was a problem hiding this comment.
ACK ab38cb0
I saw some obvious mistakes so I pushed fixes as commits. Everything else looks good to me. The LowestFee bound function looks correct.
The logic was wrong before because it was making the RBF constraints take precedence. Instead, we should be taking the maximum between the fee calculated from the target feerate and the minimum fee that satisfies the RBF constraints.
evanlinjin
force-pushed
the
no_more_base_weight
branch
from
ab38cb0 to
c650306
Compare
Fixes #1
On top of #19
Targetbut theweights were accounted for in CoinSelector. Now they're in all in
target.
therefore the actual weight of the transaction accounting for the varint for the number of outputs.
This wasn't what the issue had in mind but it was easier to take the
base_weightout ofCoinSelectorand put it inTargetrather than putTargetinCoinSelector. Getting rid ofbase_weightis a more crucial change than expected because rust bitcoin changed whatTransaction::weightreturns for empty output transactions recently so using it to determinebase_weightwill get different answers between versions (this breaks our weight tests but this PR will fix it I think if we uprade dev deps). We only need to know the total weight of the outputs and how many there are now to get the right answers for weight.