💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420969629)
The consequence is that size estimation will overestimate that input's size by 1 byte, which is up to 4 weight units of overestimation and therefore fees. However it should not be possible to accidentally set an internal input to be external as all calls to `SelectExternal` are guarded by a `!IsMine()`.

💬 furszy commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1420978430)
> We do remove the custom directory at the beginning of the test:
> https://github.com/bitcoin/bitcoin/pull/26564/files#diff-6a8ef76c60f30a6ca67d9f0e478fd02989c4b7fbc4c3116f80e13d873d5775e6R164
> so I think this restriction is still needed. I just verified that `/tmp/test_common_Bitcoin\ Core` can be a symlink, so that would allow you to use any location, right? (I could add that to the README but not sure it's worth it.)

hmm, that wasn't there before. Why we should introduce it?.

We hav
...

💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1420979726)
Great review, thank you! I cleaned up the commit history in a rebase. The [diff is minimal](https://github.com/bitcoin/bitcoin/compare/576ec3e2debdb63e142eee2c2fa00af995542f42..5d5e8cbca785db851747c5143dc1d651a1ea11e6) but in the range diff you can see the rebasing removed a commit:

`git range-diff fbcf1029a7ba47d07a4566cf1f9d2e7b4870304a 576ec3e2debdb63e142eee2c2fa00af995542f42 5d5e8cbca785db851747c5143dc1d651a1ea11e6`

Regarding naming and in-lining, I could see the two functions being n
...

🤔 murchandamus reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1773042003)
Rebased on #29037, addressed comments by @achow101 and @brunoerg

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420971102)
I dropped tracking whether the algorithm was able to exhaustively search the UTXO pool from BnB for now

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420936262)
Thanks, great suggestion. Adopted, but with turned around comparison, it needs to be less-than (I checked with a test).

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420968134)
I just need this to be as big as the largest or bigger than any of the actual groups’ input weights. Changed to `std::numeric_limits<int>::max()`

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420959370)
Inlined the `add_utxo_at_index` function

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1420947210)
Added multiplication operator in https://github.com/bitcoin/bitcoin/pull/29037, and amended here

💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#issuecomment-1847787491)
Rebased after #28894 merge, next in the line is #28987.

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420997086)
Renamed to outpoint here and elsewhere.

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420997175)
Done

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420998401)
`GetTransactionInputWeight` returns an `int64_t`, so I've unified these to use `int64_t` as well.

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420998560)
Good point. changed to use optionals.

💬 achow101 commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1420998652)
Done

💬 hebasto commented on pull request "build: Require C++20 compiler":
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1847830742)
> Looks like configure doesn't detect g++-10 as a C++20 compiler...

> I presume this will be fixed by cmake...

It [does](https://github.com/hebasto/bitcoin/pull/60) :)

💬 fanquake commented on pull request "build: Require C++20 compiler":
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1847839860)
Isn't it just applying the exact same workaround (internally) that we are using? There's no other way to know that GCC supports C++20, because of the bug we discussed above.

💬 hebasto commented on pull request "build: Require C++20 compiler":
(https://github.com/bitcoin/bitcoin/pull/28349#issuecomment-1847850612)
> Isn't it just applying the exact same workaround (internally) that we are using? There's no other way to know that GCC supports C++20, because of the bug we discussed above.

I didn't look into CMake code for that. I only tested it, including the minimum supported CMake 3.16.

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-1847884788)
After offline discussion with @sdaftuar , I've worked on a set of prospective changes and pushed them to this branch as follow-on commits.

Key change is that we will now only allow package RBF when the conflicted transactions are all in "clusters" of up to size 2. This allows us to calculate mining scores for each conflicted transaction, which means that post-cluster mempool, if we did this right, IIUC, the behavior should match with more general package RBF.

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1847943885)
Benchmark crash should be fixed.