💬 maflcko commented on pull request "Fix issues with CI on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1654199663)
> The actual code change is very simple

If it was so simple, then it would be good to explain how it could possibly work at all. Let me know what I am missing:

> However, this change may lead to problems. For example, does yaml allow duplicate keys? If yes, how are they handled? Does the skip via FILTER_TEMPLATE override this one, or vice versa? Will it change in the future?

See also "yaml duplicate keys" in your favourite search engine.

👍 maflcko approved a pull request: "Fix issues with CI on forks"
(https://github.com/bitcoin/bitcoin/pull/29274#pullrequestreview-2140849311)
lgtm ACK

My preference would be to have the `.cirrus.yml` as single source of ground truth. However, requiring forks to update this file is too cumbersome, so this alternative seems fine.

(The "config resolution strategy" is another setting outside the yaml that needs to be modified to "merge for prs", which could be documented in a follow-up)

But this looks good in any case.

💬 Sjors commented on pull request "Fix issues with CI on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1654215983)
It's conceptually simple, but I agree that we shouldn't use duplicate keys unless Cirrus docs explicitly says you can. I could work around it in various ways, but for now I found it easier to just drop the commit.

💬 maflcko commented on pull request "Fix issues with CI on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-2190935028)
> Optionally set NO_BRANCH=true ~and NO_ARM=true~ env variables (see .cirrus.yml)

This is wrong in the description. The name was changed.

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