💬 edilmedeiros commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#discussion_r1593134757)
I see you are a newcomer too. When reviewing code, better to open the files in the text editor and check for a broader context than what the github interface will present.

In my case, I use emacs with ediff, but all major IDEs will have a nice diff interface to inspect the PRs.

📝 stickies-v opened a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058)
This PR:
- moves warnings from common to the node library and into the node namespace (as suggested in https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1570069541)
- generalizes the warnings interface to `Warnings::Set()` and `Warnings::Unset()` methods, instead of having a separate function and globals for each warning. As a result, this simplifies the `kernel::Notifications` interface.
- removes warnings.cpp from the kernel library
- removes warning globals
- adds testing for the
...

💬 stickies-v commented on pull request "rpc: return warnings as an array instead of just a single one":
(https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1593174777)
> I guess it would be nice to actually move it to the node library, and node namespace?

Done in https://github.com/bitcoin/bitcoin/pull/30058

🤔 edilmedeiros reviewed a pull request: "test: use assert_greater_than util"
(https://github.com/bitcoin/bitcoin/pull/30019#pullrequestreview-2044306054)
Another thing: I have the impression that the individual commits do not work individually. If so, think about `squash`ing them all together. I don't think this will break the bank for other reviewers.

💬 edilmedeiros commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#issuecomment-2099430434)
All right, better than my review would be to convert this to a scripted diff as you are doing in #29500.

💬 andrewtoth commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2099505541)
@maciejsszmigiero why not increase `max_file_size` even more? Are there drawbacks to increasing it to 256 MB or 1 GB?

💬 tdb3 commented on pull request "test: added test coverage to loadtxoutset could not open file":
(https://github.com/bitcoin/bitcoin/pull/30053#discussion_r1593301869)
Minor nit:
Style guidelines prefer `f'{}'`. (https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines)

e.g.

```diff
diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py
index a65a11a9b5..47f87297f3 100755
--- a/test/functional/feature_assumeutxo.py
+++ b/test/functional/feature_assumeutxo.py
@@ -156,7 +156,7 @@ class AssumeutxoTest(BitcoinTestFramework):
self.log.info("Test bitcoind should fail when fi
...

🤔 tdb3 reviewed a pull request: "test: added test coverage to loadtxoutset could not open file"
(https://github.com/bitcoin/bitcoin/pull/30053#pullrequestreview-2044504789)
ACK for ee67bba76cca2355541f99bb731f58479981b29e

Thank you for increasing test coverage.
Ran a sanity check that created the .../invalid/path file from dumptxoutset and the test failed (as expected).
Left a minor nit.

💬 davidgumberg commented on pull request "test: added test coverage to loadtxoutset could not open file":
(https://github.com/bitcoin/bitcoin/pull/30053#issuecomment-2099652701)
LGTM ACK https://github.com/bitcoin/bitcoin/pull/30053/commits/ee67bba76cca2355541f99bb731f58479981b29e

This PR improves coverage of the `loadtxoutset` rpc, I tested by removing the error throw from `blockchain.cpp` and verifying that the test complains:

```diff
FILE* file{fsbridge::fopen(path, "rb")};
AutoFile afile{file};
if (afile.IsNull()) {
- throw JSONRPCError(
- RPC_INVALID_PARAMETER,
- "Couldn't open file " + path.utf8string() + " for reading.");
}
```
...

:lock: fanquake locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/30056)

💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2099665991)
https://github.com/bitcoin/bitcoin/pull/29868/checks?check_run_id=24699627882:
```bash
In file included from common/run_command.cpp:13:
./util/subprocess.h: In member function ‘int subprocess::Popen::wait()’:
./util/subprocess.h:1062:7: error: unused variable ‘ret’ [-Werror=unused-variable]
1062 | int ret = WaitForSingleObject(process_handle_, INFINITE);
| ^~~
```

🚀 fanquake merged a pull request: "doc: fix broken relative md links"
(https://github.com/bitcoin/bitcoin/pull/30025)

👍 fanquake approved a pull request: "ci: Exclude feature_init for now in valgrind task"
(https://github.com/bitcoin/bitcoin/pull/30054#pullrequestreview-2044582434)
ACK fab179d10243e85cdb172a9f08bcb7ec19ddf74d

✅ fanquake closed an issue: "test: Intermittent issue in feature_init.py", line 88, in run_test with node.wait_for_debug_log([terminate_line]): AssertionError: [node 0] Expected messages "[b'scheduler thread start']" does not partially match log:"
(https://github.com/bitcoin/bitcoin/issues/30011)

🚀 fanquake merged a pull request: "ci: Exclude feature_init for now in valgrind task"
(https://github.com/bitcoin/bitcoin/pull/30054)

📝 luke-jr opened a pull request: "Add option dbfilesize to control LevelDB target ("max") file size"
(https://github.com/bitcoin/bitcoin/pull/30059)
Debug option to control LevelDB file sizes. Since LevelDB seems to always overshoot, "max" didn't seem fitting.

Intended to be followed up with a change to the default in a rebased #30039

💬 fanquake commented on pull request "build: Require libc++-16 or later":
(https://github.com/bitcoin/bitcoin/pull/29077#issuecomment-2099704210)
> So I'd say to close this and bump to 16 wholesale?

I think generally, that would be more straightforward, then supporting varying compiler + std lib combinations, and seems possible to do at this point.

💬 fanquake commented on pull request "guix: build with glibc 2.31":
(https://github.com/bitcoin/bitcoin/pull/29987#issuecomment-2099704721)
> Is there a benefit to this? Just dropping patches?

No, it's not just dropping patches. It's about us not having to maintain an EOL branch of glibc, us getting bugfixes (if relevant) to the branch we are using, us getting closer to properly supporting hardening features, fully static builds etc, by using a glibc that supports them.

💬 maflcko commented on pull request "test: added test coverage to loadtxoutset could not open file":
(https://github.com/bitcoin/bitcoin/pull/30053#discussion_r1593521401)
With 5 ACKs this nit does not seem worth it to address at this point. Can do in one of the other pulls that touch this file, in a follow-up?

👍 S3RK approved a pull request: "Fix waste calculation in SelectionResult"
(https://github.com/bitcoin/bitcoin/pull/28366#pullrequestreview-2044816148)
Code review ACK e95b9159380f2de7f9a6e7a202cc171ad285ee6c

I'm not sure whether 44db79b0c48d6b1a26dba6ab01c2a296d610c06b is useful since it's fully replaced by the following commit.

Also added a bunch of nits in the tests about comments and potential simplifications, but nothing blocking.