💬 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
...

💬 sdaftuar commented on pull request "policy: restrict all TRUC (v3) transactions to 25KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2057668820)
> Would this make things {broken, inconvenient} for potential users of TRUCs, e.g. by requiring swapping between v3 and v2?

Just wanted to add -- I think the case I expect more often would be that users would just create multiple transactions if they had a need to construct a transaction greater than 25kvb (similar to what I think everyone must do today to work around the current 100kvb standardness limit).

The additional transaction overhead from constructing four 25kvb transactions inste
...

✅ laanwj closed an issue: "v27.0 Testing"
(https://github.com/bitcoin/bitcoin/issues/29697)

💬 laanwj commented on issue "v27.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/29697#issuecomment-2057674519)
Closing this issue now that `v27.0` final is tagged.

✅ fanquake closed an issue: "27.0 RC Testing Guide Feedback"
(https://github.com/bitcoin/bitcoin/issues/29685)

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1566348693)
Dropped `uint160`, changed to use an array.