💬 furszy commented on pull request "init: handle empty settings file gracefully":
(https://github.com/bitcoin/bitcoin/pull/29144#issuecomment-1904050547)
> nano-nit: There are some complaints from [clang-format-diff.py](https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy).

Done.

👍 hebasto approved a pull request: "init: handle empty settings file gracefully"
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1836522437)
re-ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766.

📝 Christewart opened a pull request: "Add test for negative transaction version w/ CSV to tx_valid.json"
(https://github.com/bitcoin/bitcoin/pull/29291)
This PR adds a static test vector corresponding to the bug found in various implementations of the bitcoin protocol discovered by @dergoegge

For more information see:

https://delvingbitcoin.org/t/disclosure-btcd-consensus-bugs-due-to-usage-of-signed-transaction-version/455

👋 Christewart's pull request is ready for review: "Add test for negative transaction version w/ CSV to tx_valid.json"
(https://github.com/bitcoin/bitcoin/pull/29291)

👍 ryanofsky approved a pull request: "init: handle empty settings file gracefully"
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1836698513)
Code review ACK e9014042a6bed8c16cc9a31fc35cb709d4b3c766. Just whitespace formatting changes and shortening a test string literal since last review

💬 dergoegge commented on pull request "Nuke adjusted time from validation (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1904214370)
This PR now only removes adjusted time from validation code while keeping all the code for the warning in place as is. A follow-up can change the warning (as it has many problems) but this PR will focus on the removal of adjusted time from consensus.

Re-implementing the same warning seemed unnecessary (even if it was less code) and prone to bugs.

👍 ryanofsky approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1836718832)
Code review ACK 22048c19e236e4b683f1c8192883545d5c68f793

Reflecting more on https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1458922669, I lean more towards opinion that ValidationSignals parameters passed to ChainstateManager and CTxMemPool should be optional rather than required. Two reasons:

- Validation code is a hairball of interdependencies and classes that don't really play the roles you would expect based on their names, so a change that lets them have fewer hard dependenc
...

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1462039246)
In commit "scripted-diff: Rename SingleThreadedSchedulerClient to SerialTaskRunner" (70793430bb072ceae589f4d44c49ce2c5db23216)

Harmless, but `s 'SinglethreadedSchedulerClient' 'SerialTaskRunner' ''` is repeated twice in the scripted diff script

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1462034146)
In commit "[refactor] Make MainSignalsImpl RAII styled" (35ee50f762aee78944f563b74a5e9a677980400d)

This change is moving the scheduler creation and RegisterBackgroundSignalScheduler assignment from ChainTestingSetup to BasicTestingSetup, but it is leaving the `m_service_thread` scheduler thread creation in ChainTestingSetup. This makes me wonder whether the validation_signals instance created here works, and whether it should be moved to ChainTestingSetup instead of BasicTestingSetup. That wo
...

💬 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