💬 jsarenik commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2190512170)
Tested ACK aba504759caa

🤔 kevkevinpal reviewed a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2140417645)
Concept ACK [13a871f](https://github.com/bitcoin/bitcoin/pull/30339/commits/13a871f8e3835355b84ba8a404870dcb33441aef)

ran locally and it passed for me

💬 kevkevinpal commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653913528)
I'm not sure if we test for any list of added_nodes greater than a length of 1

💬 kevkevinpal commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653905703)
instead of modifying the code here why not use `self.nodes[0].addnode(node="11.22.33.44", command='remove')` instead?

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