💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1462013455)
In commit "[refactor] Prepare for g_signals de-globalization" (4083720b33763ad2547a3e1ef9bb16cc979f5d31)

I was confused about how this was compiling given "scripted-diff: Rename MainSignals to ValidationSignals" in the previous commit. But I guess it that commit did not actually rename the GetMainSignals function.

Would maybe suggest clarifying that commit message to mention it only renames the main signals class not the function. Again, not important, but I think it would be a little bett
...

👍 dergoegge approved a pull request: "Add test for negative transaction version w/ CSV to tx_valid.json"
(https://github.com/bitcoin/bitcoin/pull/29291#pullrequestreview-1836846985)
ACK 97181decf5726aab6c5cd01b3e1964072f2531ff

💬 brunoerg commented on pull request "wallet, rpc: add BIP44 `account` in `createwallet`":
(https://github.com/bitcoin/bitcoin/pull/29129#issuecomment-1904358092)
> Also agree very much with luke-jr that it would be good not to allow this option to be passed by position, and instead to add an OBJ_NAMED_PARAMS options parameter instead. It probably makes sense to have a separate PR disallowing the other options from being passed by position as well, especially since 6 of the 7 options are bool values, so very easy to confuse if passed by position.

I strongly agree with that. I could turn this draft until we've that.

💬 vasild commented on pull request "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses":
(https://github.com/bitcoin/bitcoin/pull/26859#issuecomment-1904370264)
To reproduce: `Base64: IAEgdZ/fIAAAAAAaAAQAAUlJSUlZAAAAAALfAAAAGgAEAAH/+/5lAAAAgAAAIwCAKQ==`.

@mzumsande, right! What about this fix (`git diff -w`):

```diff
--- i/src/test/fuzz/banman.cpp
+++ w/src/test/fuzz/banman.cpp
@@ -67,18 +67,20 @@ FUZZ_TARGET(banman, .init = initialize_banman)
LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 300)
{
CallOneOf(
fuzzed_data_provider,
[&] {
CNetAddr net_
...

💬 theuni commented on pull request "Avoid non-self-contained Windows header":
(https://github.com/bitcoin-core/gui/pull/789#issuecomment-1904379392)
I don't disagree with using self-contained headers, but according to your link it should only be necessary if we don't define `WIN32_LEAN_AND_MEAN`, [which we do](https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L694).

So, this makes me think there must be some compilation units where this somehow doesn't get defined?

💬 hebasto commented on pull request "Avoid non-self-contained Windows header":
(https://github.com/bitcoin-core/gui/pull/789#issuecomment-1904383894)
> So, this makes me think there must be some compilation units where this somehow doesn't get defined?

I suppose, they might be the `qt*` stuff built by vcpkg.

📝 achow101 unlocked a pull request: "set `DEFAULT_PERMIT_BAREMULTISIG` to false"
(https://github.com/bitcoin/bitcoin/pull/28217)
The default activation of the `permitbaremultisig=0` option proposes an enhancement for the Bitcoin network. By refusing non-P2SH multisignature transactions from the outset, this modification would contribute to reducing spam attempts and maintaining a healthy decentralization by discouraging undesirable activities.

💬 freezoloto commented on issue "berkeley database failed to open database environment":
(https://github.com/bitcoin/bitcoin/issues/29286#issuecomment-1904409193)
Thank you my friend, I have already deleted my wallet and am re-uploading the blockchain :-)

💬 maflcko commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#discussion_r1462141265)
Why would the latter two be needed? Using only runtime checks/asserts would be more in line with the documentation. See: `DEBUG: Disable some optimizations and enable more runtime checking`. Also they are not supported for third-party code, see https://sqlite.org/debugging.html

Would be better to only use SQLITE_DEBUG? https://sqlite.org/compile.html#debug

💬 ryanofsky commented on pull request "depends: Update libmultiprocess library to fix C++20 macos build error":
(https://github.com/bitcoin/bitcoin/pull/29276#issuecomment-1904414778)
Updated 5dfd24581a7c5497601966a5371d8c33eabcee8a -> b8105b3ed7c97cd6545dba99d0e13c33f183e450 ([`pr/c14.1`](https://github.com/ryanofsky/bitcoin/commits/pr/c14.1) -> [`pr/c14.2`](https://github.com/ryanofsky/bitcoin/commits/pr/c14.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/c14.1..pr/c14.2)) just adding a new macos build fix: https://github.com/chaincodelabs/libmultiprocess/pull/93.

I'm pretty sure the new fix is not needed for any current depends builds because depends build
...

💬 brunoerg commented on pull request "fuzz: compare scripts from `Expand` and `ExpandFromCache`":
(https://github.com/bitcoin/bitcoin/pull/28908#issuecomment-1904416619)
Rebased

💬 brunoerg commented on pull request "test: add coverage for bech32m in `wallet_keypool_topup`":
(https://github.com/bitcoin/bitcoin/pull/28928#issuecomment-1904416959)
Rebased

💬 brunoerg commented on pull request "contrib: add test for bucketing with asmap":
(https://github.com/bitcoin/bitcoin/pull/28869#issuecomment-1904418492)
Rebased

💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1904423201)
Ok, fine. Then, just a comments nit, the recently added comments "Per BIP159, safety buffer" and "BIP159 connections safety window" better be removed? Because that is not "A safety buffer of 144 blocks to handle chain reorganizations"?

💬 hebasto commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#discussion_r1462153876)
> Would be better to only use SQLITE_DEBUG?

Thanks! Done.

💬 theStack commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#issuecomment-1904438007)
Rebased on master (to fix CI).

💬 hebasto commented on pull request "Avoid non-self-contained Windows header":
(https://github.com/bitcoin-core/gui/pull/789#issuecomment-1904455841)
@theuni

You might be interested to consider the following example: https://godbolt.org/z/b8rns1e6x

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#issuecomment-1904479665)
> In https://github.com/sipa/bitcoin/commits/pr29242 I pushed another commit which makes CompareFeerateDiagram not modify the diagrams in-place.

Taken only with minor comment changes, and added another test case or two to cover the iterative nature of the check

💬 hebasto commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#issuecomment-1904504426)
While always overriding Autotools' defaults, are there any particular reasons not using [`-O3`](https://gcc.gnu.org/onlinedocs/gcc/Optimize-Options.html#index-O3) instead of `-O2`?

👍 hebasto approved a pull request: "build: always set `-g -O2` in `CORE_CXXFLAGS`"
(https://github.com/bitcoin/bitcoin/pull/29205#pullrequestreview-1837055782)
ACK 00c1e2aa4496b5f038ae5199dbd16d8313766533, I have reviewed the code and it looks OK. Also tested `ci/test/00_setup_env_native_valgrind.sh`.