💬 edilmedeiros commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#discussion_r1593156322)
```diff
index ff67207c4e..2a4cc9dfba 100755
--- a/test/functional/p2p_segwit.py
+++ b/test/functional/p2p_segwit.py
@@ -85,6 +85,7 @@ from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
assert_greater_than,
+ assert_greater_than_or_equal,
softfork_active,
assert_raises_rpc_error,
)
@@ -376,14 +377,14 @@ class SegWitTest(BitcoinTestFramework):
self.test_node.send_message(msg_headers())


...

💬 edilmedeiros commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#discussion_r1593098839)
Why `MAX_NODES + 1`?

💬 edilmedeiros commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#discussion_r1593132011)
This is needed as this specific test is full of `Decimal` objects. This is what you get if you take it out.

```
❯ test/functional/wallet_abandonconflict.py
2024-05-07T21:36:52.495000Z TestFramework (INFO): PRNG seed is: 7791852828985173276
2024-05-07T21:36:52.495000Z TestFramework (INFO): Initializing test directory /var/folders/pk/trlzyy614z11wn_y1t8vhsz80000gn/T/bitcoin_func_test_8oozog30
2024-05-07T21:36:55.004000Z TestFramework (ERROR): Unexpected exception caught during testing
Trac
...

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