<@jnewbery> bumpfee is a command in the Bitcoin Core wallet to create an RBF transaction. It takes an unconfirmed transactions and then 'bumps' the fee on it by reducing the value of the change output
<@jnewbery> udiWertheimer: good question! When selecting coins to include in a transaction, each coin has a value, but there's also the cost associated with spending that coin, because it adds to the weight of the transaction
<ariard> Not on this PR specifically, but would you set out what process are you following when you review any PR on core, are you reading functions comment first? checking code flow change? etc
<merehap> Is there a privacy trade-off in adding the initially unselected output to bump the fee rate (i.e. mixing different outputs)? If there is, would that kind of topic come up during the PR review?
<@jnewbery> ariard: I'll read the PR description, then do a quick skim through the PR branch commit-wise to get an idea of the overall shape of the changes, and then go through each commit in more detail
<merehap> When bumping the fee rate and including a new output, you are linking previously unlinked outputs (by my understanding). Probably not important to go into depth here though.
<udiWertheimer> merehap: unfortunately there’s usually some privacy-related trade off with bumping fees. Even if you don’t add an input, you will expose which of the outputs is your change output (the one that changes), which otherwise would be harder to figure out
<@jnewbery> It's a good consideration. Whenever you spend multiple inputs in the same transaction, you're revealing some information about the ownership of those coins (module coinjoin, P2EP, etc). That's true for normal transactions and bumped transactions
<harding> merehap: ah. That's an interesting question. I think it didn't come up in the review because linknig together inputs in inherent in sending transactions at all, whether the user is sending a new transaction or bumping the fee of an existing transaction.
<@jnewbery> bumpfee was added a couple of years ago and had pretty good test coverage, so it was pretty straightforward for instagibbs to add new tests
<MrPaz> so if I understand correctly, before you couldn't increase fee because there weren't enough sats in input and there was simply no way to add a new input. now there is a way to add new input to bump fee, and it will look for uneconomic inputs to use in bumping fee?
<ecurrencyhodler> karimofthecrop: bumpfee is a command in the Bitcoin Core wallet to create an RBF transaction. It takes an unconfirmed transactions and then 'bumps' the fee on it by reducing the value of the change output previously that command would fail if there wasn't a change output, or the change output was too small this PR allows the fee to be bumped on transactions without change or with small change by adding a n
<@jnewbery> MrPaz: it will look for any inputs to add to the transactions. It just happens that it might pick uneconomical inputs as part of that knapsack search
<harding> mryandao: I don't think that's something this PR changes, as it's Bitcoin Core's existing wallet policy to not send outputs that may cost more to spend than they're worth.
<rafeeki> jnewbery: thanks! I liked the idea of starting to contribute just by running/writing test cases. It surprised me that this one was so formulaic. I guess in future weeks we will see other examples that may need more testing?
<harding> rafeeki: I think this PR probably could've used more tests, and contributions to testing are welcome independently from the PRs that implement the features.
<aj> harding: while you're reviewing adding tests yourself help you understand the behaviour, and you can send them to the author who can add them to the PR too. super helpful in my opinion
<@jnewbery> refeeki/harding: manual testing of new features is always welcome. And like aj says, contributing automated tests to the PR author is a really helpful way to start contributing
<dmkathayat> Not specific to this PR, but do you review PRs directly on github or on your terminal? I'm finding it tricky to just read through the line changes on github.
<moneyball> bilthon: concept ACK just means that the reviewer acknowledges and agrees with the concept of the change, but is not (yet) confirming they've looked at the code or tested it. this can be a valuable signal to a PR author to let them know that the PR has merit and is headed in the right direction.
<fanquake> ariard I have some tools, info & random stuff in https://github.com/fanquake/core-review. Has guides on how to gitian build, review certain types of PRs etc.
<@jnewbery> instagibbs: indeed. If you want to fully remove trust, you can go the full MarcoFalke and sign/opentimestamp all of your review comments :)
<@jnewbery> kcalvinalvin: it's a good question. In this case, the changes are all at a higher application-level, so very unlikely to have portability issues
<fanquake> Also depends on what the expectations are in regards to building with provided libraries or depends. i.e qt is somewhat broken on debian atm: https://github.com/bitcoin/bitcoin/issues/15453
<@jnewbery> I'm not sure if there's a quick way to identify which PRs could use additional testing without looking at them. In general, I'd recommend that you pick one subsystem that you're interested in (eg wallet, testing framework, rpc, net), and try to survey all the open PRs in that area, then identify which ones you think need additional testing/review
<harding> karimofthecrop: there's also https://gist.github.com/jnewbery/6e2797a6f484de59aefc849a6b184008 , which has the PRs planned to be discussed here in the next couple weeks. I don't know if any of them need more tests written, but it'd be worth checking.
<@jnewbery> or feel free to ask in #bitcoin-core-dev. An "I'm interested in helping test wallet/net/etc PRs and I'm looking for PRs to help on" would definitely be well received
<@jnewbery> fanquake: yes, those are high-priority for review PRs. You should expect to see more action on those than the average PR. Definitely worth knowing what's on there and what other people are looking at
<@jnewbery> karimofthecrop: if a PR has needed rebase for a while (say more than a week or so), then I interpret it as the author not actively working on it, so I tend to avoid review until it's more active
<@jnewbery> Feedback is most definitely welcome. Also let me know if you have suggestions for PRs to cover. The aim is for manageable sized PRs (ie 100-150 LOC change seems about right), not too much contextual knowledge required, and trying to cover all the different components