💬 ajtowns commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1903768404)
> I'm somewhat puzzled why Knots (cc @luke-jr) and Inquisition (cc @ajtowns) are not running into the same issues. But I guess there's at least two differences:

Inquisition disabled all the automatic tests except the trivial linter when cirrus imposed limits earlier in the year, and tests are only run manually by reviewers. Haven't worked out what it should be doing, so don't have suggestions to offer, but interested to see what the resolution here is.

💬 willcl-ark commented on issue "berkeley database failed to open database environment":
(https://github.com/bitcoin/bitcoin/issues/29286#issuecomment-1903822925)
Hello @freezoloto. Can you confirm that there is a wallet database directory at that path? This error is primarily thrown when that directory does not exist, but can be thrown for other reasons, so it would be good to confirm that it does exist before trying to investigate further.

On Linux (or WSL) this could be done by running something like this:

```bash
will in ~ :
$ cd .bitcoin

will in ~/.bitcoin:
$ ls -al
.rw------- 75 will 22 Jan 09:15 .cookie
.rw------- 0 will 12 Jan 1
...

💬 moemat12 commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#discussion_r1461745349)
src/wallet/sqlite.h

💬 dergoegge commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1461750521)
> CConMan cannot access the chain state, the sync progress, in-flight block requests

I don't see how the connection opening logic needs access to any of those things.

> number of compact block filters

Bitcoin Core doesn't download compact block filters.

> v2 encrypted connections, etc.

Connman is doing this right now using our service flags and the service flag of the potential new peer. It has all the data it needs.

> Essentially, in my view, the approach is resource-wasteful
...

✅ jerzybrzoska closed a pull request: "Update build-unix.md - add boost library"
(https://github.com/bitcoin/bitcoin/pull/29290)

💬 jerzybrzoska commented on pull request "Update build-unix.md - add boost library":
(https://github.com/bitcoin/bitcoin/pull/29290#issuecomment-1903861175)
> The docs suggest to install or build boost one line below already:
>
> https://github.com/bitcoin/bitcoin/blob/651fb034d85eb5db561bfd24b74f7271417defa5/doc/build-unix.md?plain=1#L49-L51
>
> Does that not work for you?

Yes it does.

👍 hebasto approved a pull request: "init: handle empty settings file gracefully"
(https://github.com/bitcoin/bitcoin/pull/29144#pullrequestreview-1836318583)
ACK d664eab66ecfa1d318b11de5deafb04b1e3c4c06, tested on Ubuntu 23.10.

nano-nit: There are some complains from [`clang-format-diff.py`](https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy).

💬 ismaelsadeeq commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1461755517)
Specify the restrictions is only for unconfirmed v3 transaction here and other places.
```suggestion
// v3 only allows 1 unconfirmed parent and 1 unconfirmed child.
```

🤔 ismaelsadeeq reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1836296374)

> I've consolidated the rules in the BIP ([bitcoin/bips#1541](https://github.com/bitcoin/bips/pull/1541)), agree it's easier to understand this way.

I've looked at the BIP and this implementation matches the specification in the BIP

> I'll delete or clean up the version3_transactions.md doc here now that the information has been moved to the BIP.

+1

Changes since my [last review](https://github.com/bitcoin/bitcoin/compare/1dd62c3df4856c36bfc610f700684852772dd9f7..f3d4916eacfbcef61b
...

💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1903992967)
> > I haven't looked in detail, but writing bytes to a file can be achieved with one line of code
>
> That would be a nice simplification. Almost (?) to the point of not needing these helper functions.

WriteBinaryFile is unused right now either way outside of tests, so I guess this could be removed regardless, as future code can just use the in-line one-liner?

💬 furszy commented on pull request "script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks)":
(https://github.com/bitcoin/bitcoin/pull/28923#discussion_r1461879554)
Would suggest to use `FlatSigningProvider` instead (see #28307).
It will also save you one extra `GetScriptForDestination` call per created key.

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