💬 achow101 commented on issue "26.0 - Released binaries hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/29038#issuecomment-1847686434)
1. The github download links are automatically generated by github. They are not built with the deterministic build process and we do not use them for the published releases. The warning in the github release explicitly says not to use them.
2. This link points to the tarball containing the binaries for x86_64 linux systems, whereas the hash you have posted is for the source tarball. Notice how the names are different.

💬 RocketNodeLN commented on issue "26.0 - Released binaries hash mismatch":
(https://github.com/bitcoin/bitcoin/issues/29038#issuecomment-1847691317)
https://bitcoincore.org/bin/bitcoin-core-26.0/bitcoin-26.0-x86_64-linux-gnu.tar.gz is not the source tarball

💬 kevkevinpal commented on pull request "test: detect OS in functional tests consistently using `platform.system()`":
(https://github.com/bitcoin/bitcoin/pull/29034#issuecomment-1847703106)
ACK [878d914](https://github.com/bitcoin/bitcoin/pull/29034/commits/878d914777a03a04ecb84217152e8b7fd73a5062)

---

Ran this command to make sure we were not missing any, only seeing os.name show up for posix in `test_framework.py`
```
$ grep -nr "os\.name" ./test && grep -nr "sys\.platform" ./test

./test/functional/README.md:40:- Use `platform.system()` for detecting the running operating system and `os.name` to
./test/functional/test_framework/test_framework.py:918: if os.name
...

💬 ajtowns commented on pull request "build: Enable -Wunreachable-code":
(https://github.com/bitcoin/bitcoin/pull/28999#issuecomment-1847739799)
> ACK [fa8adbe](https://github.com/bitcoin/bitcoin/commit/fa8adbe7c17b16cf7ecd16eb9f3f1792e9da2876)
>
> At least on my machine, this does't catch [#28830 (comment)](https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1415690508) though.

`Assert(!error)` in that case is trivially true (error was just set to nullopt, !nullopt is true), so presumably it's getting compiled to a no-op, at which point there is no unreachable code to warn about?

💬 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) :)