📝 fanquake opened a pull request: "guix: use GCC 13 to builds releases"
(https://github.com/bitcoin/bitcoin/pull/29881)
Based on #29828 and #29846 (Windows cleansup, please review first).

Also requires a bump to the Guix time-machine, to pull in the relevant Windows fixups:
* https://git.savannah.gnu.org/cgit/guix.git/commit/?id=ffd1d0c2d486b9de35706ca73a4e89177114e4de
* https://git.savannah.gnu.org/cgit/guix.git/commit/?id=97bad5e1947a07d96f40205bec1e91463e7bf014

Otherwise, is enough to switch release builds to using GCC 13.2.0: https://gcc.gnu.org/gcc-13/.

Does not solve the cross-arch non-determinis
...

💬 achow101 commented on pull request "bumpfee: ignore WALLET_INCREMENTAL_RELAY_FEE when user specifies fee_rate":
(https://github.com/bitcoin/bitcoin/pull/27969#issuecomment-2057413771)
ACK f58beabe754363cb7d5b24032fd392654b9514ac

💬 naiyoma commented on pull request "test: Refactor fee calculation to remove satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2057426877)
> tACK [b7a4a81](https://github.com/bitcoin/bitcoin/pull/29566/commits/b7a4a8118b43ebd2ab1b64247990ff49d6f19d26)
>
> Make successful, all functional tests successful.
>
> > To address this, the satoshi_round function is removed, and Decimal values are utilized in its place.
>
> 1. As per the second commit, the `satoshi_round` is back, let's update the PR description to reflect that because it will be picked up by the bot on merge?
> 2. Should we consider squashing the first 2 commits i
...

🤔 stickies-v reviewed a pull request: "kernel: De-globalize fReindex"
(https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2001088868)
Approach ACK d9bcbbf2293ef427b37eefca30f074be5eeeca26 and code LGTM.

---

Some thoughts I have from reviewing this PR, but that are orthogonal to it:

It seems like we have 4 places where we store information about reindexing being required or being in progress:
1. `BlockTreeDB` (through `WriteReindexing()` and `ReadReindexing()`)
1. `BlockManager::m_reindex` (previously `fReindex`)
1. `BlockManagerOpts::reindex`
1. `ChainstateLoadOptions::reindex`

1\. and 2. are updated based on r
...

💬 stickies-v commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1565782441)
To avoid defining the default value twice (default member init in `BlockManagerOpts`), perhaps better to use:

```suggestion
if (auto value{args.GetBoolArg("-reindex")}) opts.reindex = *value;
```

💬 laanwj commented on pull request "tracing: cast block_connected duration to µs":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2057427995)
Code review ACK ca4eaa5777888292bbdef3ae8d45af6a407df5df

💬 pinheadmz commented on pull request "netbase: remove unnecessary log message":
(https://github.com/bitcoin/bitcoin/pull/29649#issuecomment-2057446510)
> far less frequent

Those logs should only print when the users local proxy is unreachable, which should be less frequent and also a bit more justified. However it is also spammy in that case. I ran with `-proxy=unix:/path/to/tor/socket` and then after a minute, shut down the Tor daemon. Do you think this should be cleaned up as well?

```
2024-04-15T17:22:45Z connect() to 127.0.0.1:7656 failed after wait: Connection refused (61)
2024-04-15T17:22:47Z [error] Error while reading proxy resp
...

💬 achow101 commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#issuecomment-2057453786)
ACK 3a3ccf00f0bb6b6450dcf859982cdb40bb06f475

💬 pinheadmz commented on pull request "netbase: remove unnecessary log message":
(https://github.com/bitcoin/bitcoin/pull/29649#issuecomment-2057454163)
Although actually the unix sockets PR didnt change this behavior much. If you start v26 with a black hole like `-proxy=127.0.0.1:9999` you get similar spam:

```
2024-04-15T17:28:12Z connect() to 127.0.0.1:7656 failed after wait: Connection refused (61)
2024-04-15T17:28:12Z connect() to 127.0.0.1:9999 failed after wait: Connection refused (61)
2024-04-15T17:28:13Z connect() to 127.0.0.1:7656 failed after wait: Connection refused (61)
2024-04-15T17:28:13Z connect() to 127.0.0.1:9050 failed
...

👍 stickies-v approved a pull request: "test: Add missing Assert(mock_time_in >= 0s) to SetMockTime"
(https://github.com/bitcoin/bitcoin/pull/29872#pullrequestreview-2001765599)
ACK fae0db555c12dca75fb09e5fa7bbabdf39b8c1df

💬 stickies-v commented on pull request "test: Add missing Assert(mock_time_in >= 0s) to SetMockTime":
(https://github.com/bitcoin/bitcoin/pull/29872#discussion_r1566195570)
nit: any reason to not just use `assert`?

💬 fanquake commented on pull request "netbase: remove unnecessary log message":
(https://github.com/bitcoin/bitcoin/pull/29649#issuecomment-2057456834)
> Do you think this should be cleaned up as well?

Looks like that could be a good idea.

💬 achow101 commented on pull request "sign: don't assume we are parsing a sane TapMiniscript":
(https://github.com/bitcoin/bitcoin/pull/29853#discussion_r1566217127)
I think it would be good to check that `SignSignature` fails here:

```suggestion
BOOST_CHECK(!SignSignature(keystore, CTransaction(prev), curr, 0, SIGHASH_ALL, sig_data));
```

💬 laanwj commented on issue "Intermittent issue in feature_proxy.py AssertionError: not(bytearray(b'node.noumenon') == b'fc00:1:2:3:4:5:6:7')":
(https://github.com/bitcoin/bitcoin/issues/29871#issuecomment-2057480445)
Looks like the two successive testcases in `node_test` are out of step? `[fc00:1:2:3:4:5:6:7]:8888` is from the CJDNS test, `node.noumenon:8333` is the name-based test after it, interfering with each other.

🤔 achow101 reviewed a pull request: "rpc: parse legacy pubkeys consistently with specific error messages"
(https://github.com/bitcoin/bitcoin/pull/28336#pullrequestreview-2001820644)
ACK 98570fe29bb08d7edc48011aa6b9731c6ab4ed2e

💬 achow101 commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1566227161)
In 100e8a75bf5d8196c005331bd8f2ed42ada6d8d0 "rpc: check and throw specific pubkey parsing errors in `HexToPubKey`"

nit: The error message reads a little bit weirdly to me. I think it would make more sense to use "is not" rather than "must be", e.g. "Pubkey abcd is not a hex string".

💬 achow101 commented on pull request "rpc: validate fee estimation mode case insensitive":
(https://github.com/bitcoin/bitcoin/pull/29175#issuecomment-2057510602)
> This is probably more than you wanted to do, but I'd say it would be better to use `FeeModeFromString` to re-use the existing (case-insensitive) parsing code, than to re-implement the parsing and use hard-coded "unset" strings in all call places.

I agree. I think it would make more sense for `FeeModeFromString` to handle "unset" and to check its return value rather than having these repeated string comparisons. It's a bit odd that the estimate mode is being validated in 3 places - `Interpre
...

📝 pinheadmz opened a pull request: "netbase: clean up Proxy logging"
(https://github.com/bitcoin/bitcoin/pull/29882)
Follow up to #27375 and see https://github.com/bitcoin/bitcoin/pull/29649#issuecomment-2057456834

This removes an extra log message when we can't connect to our own proxy. It also removes a log message if the Proxy is invalid, which is unreachable code because we check that during init by calling `SetProxy()`:

https://github.com/bitcoin/bitcoin/blob/d1af4422d13ba84253a1555c2fe5a42170fa8b35/src/netbase.cpp#L663-L666

## Before #27375 if proxy is unreachable

```
2024-04-15T17:54:51Z co
...

💬 alfonsoromanz commented on pull request "Bugfix: bitcoin-cli: Check length of peer.transport_protocol_type":
(https://github.com/bitcoin/bitcoin/pull/29657#issuecomment-2057612784)
ACK c3e632b44153e314ef946f342c68c2758b1cbc4d

⚠️ laanwj opened an issue: "utils: wallet_dump can create a `database` directory, cross-pollinating records"
(https://github.com/bitcoin/bitcoin/issues/29883)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

I created a large directory with wallet dat files for testing #26606. Ran into a weird issue where the output mismatches, but after investigating deeper, this was not due to a bug in that PR.

So I created a script that iterates over all wallets, dumps them with the internal bdb and external bdb, diffs the textual output, stopping on the first wallet which mismatches. Mind that the walle
...