💬 fanquake commented on pull request "doc: Show how less noisy clang-tidy output can be achieved":
(https://github.com/bitcoin/bitcoin/pull/27205#discussion_r1132543346)
Think you forgot to drop this?

👍 ryanofsky approved a pull request: "util: fix argsman dupe key error"
(https://github.com/bitcoin/bitcoin/pull/27236)
Code review ACK 8fcbdadfad8a9b3143527141ff37e5fe1e87f3b3. Thanks for the fix! I would maybe update the PR description or add to it to describe behavior change of this PR:

---

bugfix: Fix broken GUI "Reset" button when settings.json contains duplicate keys

Make GUI "Settings file could not be read. Do you want to reset settings to default values?" dialog actually clear all settings instead of partially keeping them when `settings.json` contains duplicate keys. This change has no effect o
...

💬 TheCharlatan commented on pull request "doc: Show how less noisy clang-tidy output can be achieved":
(https://github.com/bitcoin/bitcoin/pull/27205#issuecomment-1464014496)
Updated 3d524c9d4664026c318c2ec90baacc2bee4f4525 -> 54c4d03578c5842f19bf8bc68aca5faf8beed5c3 ([dev-notes-clang-tidy_2](https://github.com/TheCharlatan/bitcoin/tree/dev-notes-clang-tidy_2) -> [dev-notes-clang-tidy_3](https://github.com/TheCharlatan/bitcoin/tree/dev-notes-clang-tidy_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/dev-notes-clang-tidy_2..dev-notes-clang-tidy_3)) to get rid of the old documentation paragraph @fanquake .

👍 ryanofsky approved a pull request: "refactor: Consistenly use context args over gArgs in node/interfaces"
(https://github.com/bitcoin/bitcoin/pull/27239)
Code review ACK facbb444bf5aea2bbaa4a4246a8a2c661d9bf314. Thanks for the cleanup. Could s/consistenly/consistently/ too

💬 ryanofsky commented on pull request "refactor: Consistenly use context args over gArgs in node/interfaces":
(https://github.com/bitcoin/bitcoin/pull/27239#discussion_r1132558283)
In commit "refactor: Consistenly use context args over gArgs in node/interfaces" (11110f2e3d97d7d88ada1fdaab70a866bf9a35d7)

Slight preference for calling this `args` not `argsman`. `args` is used more commonly in code (921 vs 340 times before this commit). Also I always thought the name `ArgsManager` was a misnomer, because it's really a data container that doesn't manage any significant internal state.

💬 brunoerg commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1464036406)
perhaps we could also check this is a hidden RPC like `addpeerinfo`?

```py
self.log.debug("Test that addpeerinfo is a hidden RPC")
# It is hidden from general help, but its detailed help may be called directly.
assert "addpeerinfo" not in node.help()
assert "addpeerinfo" in node.help("addpeerinfo")
```

💬 MarcoFalke commented on pull request "doc: Show how less noisy clang-tidy output can be achieved":
(https://github.com/bitcoin/bitcoin/pull/27205#issuecomment-1464052726)
utACK 54c4d03578c5842f19bf8bc68aca5faf8beed5c3

💬 MarcoFalke commented on pull request "refactor: Consistently use context args over gArgs in node/interfaces":
(https://github.com/bitcoin/bitcoin/pull/27239#discussion_r1132594790)
thx, done

👍 ryanofsky approved a pull request: "refactor: Consistently use context args over gArgs in node/interfaces"
(https://github.com/bitcoin/bitcoin/pull/27239)
Code review ACK fa3e9b420fbc1ce9b20218f03356502e1b79d5ff

💬 willcl-ark commented on pull request "util: fix argsman dupe key error":
(https://github.com/bitcoin/bitcoin/pull/27236#issuecomment-1464067161)
Thanks @ryanofsky . I updated the PR description with your recommendation. Did you think I should update the commit title to "bugfix: Fix broken GUI "Reset" button when settings.json contains duplicate keys" also? I'm happy to do that if so.

🚀 fanquake merged a pull request: "doc: Show how less noisy clang-tidy output can be achieved"
(https://github.com/bitcoin/bitcoin/pull/27205)

💬 ishaanam commented on pull request "test: fix race condition in encrypted wallet rescan tests":
(https://github.com/bitcoin/bitcoin/pull/27199#discussion_r1132608578)
This should be fixed now, thanks.

💬 ryanofsky commented on pull request "util: fix argsman dupe key error":
(https://github.com/bitcoin/bitcoin/pull/27236#issuecomment-1464076968)
> Thanks @ryanofsky . I updated the PR description with your recommendation. Did you think I should update the commit title to "bugfix: Fix broken GUI "Reset" button when settings.json contains duplicate keys" also? I'm happy to do that if so.

Either way seems fine, it's just a choice of whether you think it's more useful to describe the goal of the PR or the implemenation of the PR. Another alternative to changing the title could be changing the first line "fixes #22638" to "fixes #22638, br
...

💬 TheCharlatan commented on pull request "refactor: Consistently use context args over gArgs in node/interfaces":
(https://github.com/bitcoin/bitcoin/pull/27239#issuecomment-1464091061)
re-ACK [facbb44](https://github.com/bitcoin/bitcoin/pull/27239/commits/facbb444bf5aea2bbaa4a4246a8a2c661d9bf314)

💬 brunoerg commented on pull request "p2p: set `-dnsseed` and `-listen` false if `maxconnections=0`":
(https://github.com/bitcoin/bitcoin/pull/26899#issuecomment-1464096015)
Rebased

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1132629513)
Let me elaborate why I think comparing 1. and 2. above is important (the benchmarks in this PR don't do that):

* Right now, on `master`, when we pick a peer to connect to 1. happens.
* With #27213 when we pick a peer to connect to 2. happens.
* I assume most of the addrmans out there are full (tens of thousands of addresses, max 80k), thus testing on an empty addrman is not representative.

So, we will add security at the cost of making it a bit slower. To assess how much slower I tweaked
...

💬 pablomartin4btc commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#discussion_r1132681562)
thanks

💬 pablomartin4btc commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#discussion_r1132681928)
fixed it

👍 furszy approved a pull request: "Mask values on Transactions View"
(https://github.com/bitcoin-core/gui/pull/708)
ACK 4492de1

💬 mzumsande commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1132740863)
This looks like a nice speedup in the case where we only have a few addresses - however, due to the constant overhead of building the shuffled list of buckets in the beginning I'd expect performance to go down a bit for the case where we have many addresses to choose from. Do you see this in your benchmark?
Why does this need `ALREADY_VISITED_AND_BORING`? If we do a for-loop through a pre-shuffled list of buckets instead of a while loop, doesn't that already guarantee that we visit each bucket
...