💬 fanquake commented on pull request "Year update":
(https://github.com/bitcoin/bitcoin/pull/27240#issuecomment-1463857356)
Thanks. However these changes are done using a script, only at certain times.

💬 fanquake commented on pull request "refactor: Split logging utilities from system.h":
(https://github.com/bitcoin/bitcoin/pull/27238#issuecomment-1463871792)
Concept ACK

💬 Sjors commented on pull request "assumeutxo: keep cache when flushing snapshot (#17487 followup)":
(https://github.com/bitcoin/bitcoin/pull/27008#issuecomment-1463883934)
From my testing of #15606 - without this PR - I get the impression that flushing isn't working, specifically that RAM is not freed up. It could just be confusion on my end from rebase hell, but I'd like to make sure we're not obscuring a bug by merging this.

💬 fanquake commented on pull request "refactor: Consistenly use context args over gArgs in node/interfaces":
(https://github.com/bitcoin/bitcoin/pull/27239#issuecomment-1463886234)
Concept ACK

💬 furszy commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#discussion_r1132459068)
yeah good, fixed.
also, pushed an early return if the sum of the not accepted groups decreases the available amount below the target (we simply don't have enough balance after applying the filters, so no need to run the selection algos).

👋 dergoegge's pull request is ready for review: "Avoid integer overflow in CheckDiskSpace"
(https://github.com/bitcoin/bitcoin/pull/27235)

💬 furszy commented on pull request "wallet: return error msg for "too-long-mempool-chain"":
(https://github.com/bitcoin/bitcoin/pull/24845#issuecomment-1463907731)
Thanks @S3RK!, feedback tackled.

We now check, and return early, if there is no enough available balance after filtering groups and before running the selection algos.

💬 MarcoFalke commented on pull request "Avoid integer overflow in CheckDiskSpace":
(https://github.com/bitcoin/bitcoin/pull/27235#discussion_r1132473107)
```suggestion
uint64_t assumed_chain_bytes{chainparams.AssumedBlockchainSize() * 1024 * 1024 * 1024};
```

Instead of "size", use "bytes", like in other places, for example `additional_bytes_needed`?

💬 MarcoFalke commented on pull request "Avoid integer overflow in CheckDiskSpace":
(https://github.com/bitcoin/bitcoin/pull/27235#issuecomment-1463915543)
Should be trivial to add a test, no?

💬 furszy commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#discussion_r1132487327)
tiny nit:
Extra whitespace space.

💬 furszy commented on pull request "Mask values on Transactions View":
(https://github.com/bitcoin-core/gui/pull/708#discussion_r1132491161)
This is not needed. With line 17 is enough, the include is done in the cpp file.

💬 fanquake commented on pull request "rpc: fix logging RPC when "none" values are passed, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#issuecomment-1463949565)
> The behavior was intended to be added in https://github.com/bitcoin/bitcoin/pull/11191.

It's still not clear if this was broken from the time it was merged, or at some other later point. This also does not work with 22.x, and any earlier versions are EOL.

If it's been broken forever (or at least is in all currently maintained releases), and no ones even noticed, we could take advantage of that, to remove some of the complexity here; do we definitely need multiple different ways of achiev
...

💬 TheCharlatan commented on pull request "doc: Show how less noisy clang-tidy output can be achieved":
(https://github.com/bitcoin/bitcoin/pull/27205#issuecomment-1463951556)
Updated 6a29f34ac0afce501af45097916256a9bffe8d19 -> d880c78e708d3cb0c44631e3be759d6991d5a910 ([dev-notes-clang-tidy_0](https://github.com/TheCharlatan/bitcoin/tree/dev-notes-clang-tidy_0) -> [dev-notes-clang-tidy_1](https://github.com/TheCharlatan/bitcoin/tree/dev-notes-clang-tidy_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/dev-notes-clang-tidy_0..dev-notes-clang-tidy_1)) to address @MarcoFalke's feedback.

💬 MarcoFalke commented on pull request "Avoid integer overflow in CheckDiskSpace":
(https://github.com/bitcoin/bitcoin/pull/27235#issuecomment-1463966293)
Introduced in 6630a1e8448c633e4abaa8f5903f11cac6f433a7 so I guess no backport needed?

💬 TheCharlatan commented on pull request "doc: Show how less noisy clang-tidy output can be achieved":
(https://github.com/bitcoin/bitcoin/pull/27205#issuecomment-1463986021)
Updated https://github.com/bitcoin/bitcoin/commit/6a29f34ac0afce501af45097916256a9bffe8d19 -> 3d524c9d4664026c318c2ec90baacc2bee4f4525 ([dev-notes-clang-tidy_1](https://github.com/TheCharlatan/bitcoin/tree/dev-notes-clang-tidy_1) -> [dev-notes-clang-tidy_2](https://github.com/TheCharlatan/bitcoin/tree/dev-notes-clang-tidy_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/dev-notes-clang-tidy_1..dev-notes-clang-tidy_2)) to fix typos and trailing newline.

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