💬 vasild commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#issuecomment-2190939350)
`45f4dbe484...655a2cf666`: the previous push resolved the merge conflict in a too late commit, causing the "test each commit" CI job to fail

💬 Sjors commented on pull request "Fix issues with CI on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2190943848)
@maflcko fixed in description

💬 maflcko commented on pull request "Docs improvements":
(https://github.com/bitcoin/bitcoin/pull/30337#discussion_r1654225869)
wrong: `DEPENDS_DIR` is the "depends dir".

🤔 1alexbc1 reviewed a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340#pullrequestreview-2140900556)
Yea

💬 nnsW3 commented on pull request "Docs improvements":
(https://github.com/bitcoin/bitcoin/pull/30337#issuecomment-2190969315)
solve the inconvenience

💬 maflcko commented on pull request "[test]: raise an exception in `_bulk_tx_` when `target_weight` is too low.":
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1654301777)
style nit: Could use the more specific `RuntimeError` to clarify this is a programming error?

💬 maflcko commented on pull request "[test]: raise an exception in `_bulk_tx_` when `target_weight` is too low.":
(https://github.com/bitcoin/bitcoin/pull/30322#discussion_r1654301973)
same

👍 maflcko approved a pull request: "[test]: raise an exception in `_bulk_tx_` when `target_weight` is too low."
(https://github.com/bitcoin/bitcoin/pull/30322#pullrequestreview-2140994917)
lgtm

💬 ajtowns commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2191074487)
> Take a block file linearizer replacing the scripts in `contrib/linearize` as an example; you have one that reads blocks in their indexed order and another that writes blocks and their index back to another location on disk.

I don't think we should be reworking logging in our entire codebase in order to have a slight improvement in the hypothetical case where we replace some python scripts in contrib with C++ code? If this is worth doing, surely there is a *much* better example of why.

>
...

🤔 maflcko reviewed a pull request: "Mining interface followups"
(https://github.com/bitcoin/bitcoin/pull/30335#pullrequestreview-2141051831)
`CHECK_NONFATAL` can be used to write shorter code and not pollute the scope with single-use symbols. That is:

```cpp
std::optional<uint256> maybe_tip{miner.getTipHash()};
CHECK_NONFATAL(maybe_tip);
uint256 tip{maybe_tip.value()};
```

can be:

```cpp
uint256 tip{CHECK_NONFATAL(miner.getTipHash()).value()};

💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654345783)
Same in line 374

💬 maflcko commented on pull request "Mining interface followups":
(https://github.com/bitcoin/bitcoin/pull/30335#discussion_r1654338332)
What is this lock used for?

🤔 maflcko requested changes to a pull request: "Docs improvements"
(https://github.com/bitcoin/bitcoin/pull/30337#pullrequestreview-2141087191)
Other changes are also still wrong, see https://github.com/bitcoin/bitcoin/pull/30337#issuecomment-2189405730

Please make sure all changes are correct, then please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 laanwj commented on pull request "chainparams: Add achow101 DNS seeder":
(https://github.com/bitcoin/bitcoin/pull/30007#issuecomment-2191138604)
ACK 2721d64989c2b2114890586b7efd01ab4b062ca6
Adding a DNS seed adds redundancy, so does spreading them over as many top-level domains as possible, so i don't think "this domain might get blocked" is ever an argument against this.

💬 laanwj commented on pull request "rest: don't copy data when sending binary response":
(https://github.com/bitcoin/bitcoin/pull/30321#issuecomment-2191157177)
re-ACK 1556d21599a250297d5f20e5249c970340ab08bc

🤔 hebasto reviewed a pull request: "depends: build libevent with CMake"
(https://github.com/bitcoin/bitcoin/pull/29835#pullrequestreview-2141182567)
My Guix build:
```
x86_64
d702d02df48bc540da55c47ca7110d122a27ba179ab728fb8bdb6e27589f754c guix-build-f59e9057e2aa/output/aarch64-linux-gnu/SHA256SUMS.part
d806e1994bc873a975714ce75bca87fd3fcec4055e24d1ed0afe99ebc3503288 guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu-debug.tar.gz
a9844e6a4d3e86df69b98281c942ac01136b4b115fc6a2504740368c5d441fbd guix-build-f59e9057e2aa/output/aarch64-linux-gnu/bitcoin-f59e9057e2aa-aarch64-linux-gnu.tar.gz
4e3ea3b82
...

💬 laanwj commented on pull request "build: Drop redundant `sys/sysctl.h` header check":
(https://github.com/bitcoin/bitcoin/pull/30327#issuecomment-2191181941)
ACK c0b5ea5901d0ed005bca345e5b2de8a502f6af75
verified there is no `HAVE_SYS_SYSCTL_H` in the code, nor ever was, while it was introduced at the same time as `HAVE_SYSCTL`

👍 TheCharlatan approved a pull request: "depends: build libevent with CMake"
(https://github.com/bitcoin/bitcoin/pull/29835#pullrequestreview-2141199689)
ACK f59e9057e2aa596b54cf9e85bab35c3ead137547

💬 brunoerg commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1654435108)
`added_node_0` seems weird because it can be confused with nodes[0], I prefer `first_added_node`. I will address it.

💬 brunoerg commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1654436832)
Because I wanted to remove exactly the first added node.