🤔 tdb3 reviewed a pull request: "test: add coverage for `node` field of `getaddednodeinfo` RPC"
(https://github.com/bitcoin/bitcoin/pull/30339#pullrequestreview-2140194570)
Approach ACK

Good update to exercise the filtering feature of the RPC.
Left a suggesetion and nit.

💬 tdb3 commented on pull request "test: add coverage for `node` field of `getaddednodeinfo` RPC":
(https://github.com/bitcoin/bitcoin/pull/30339#discussion_r1653788093)
Manually induced a failure here by using `getaddednodeinfo(node="192.168.99.99")`. Test failed as expected.

📝 kevkevinpal opened a pull request: "test: Added coverage to Block not found error using gettxoutsetinfo"
(https://github.com/bitcoin/bitcoin/pull/30340)
#### Description
There were no tests that checked for the `Block not found` error called in `ParseHashOrHeight` when using `gettxoutsetinfo`, this change adds coverage to it.

You can see there are no tests that do the following by doing the below
`grep -nri "Block not found.**gettxoutsetinfo" ./test/functional/`

which leads to no results

---

#### Note:
I moved the order of when `self._test_gettxoutsetinfo` is called because at the end we restart the node with `-coinstatsindex=1` b
...

💬 am-sq commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1653823679)
Made the change.

💬 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