💬 paplorinc commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1593117445)
Good idea, done

💬 iotamega commented on issue "Possible to Ban Clients by Name?":
(https://github.com/bitcoin/bitcoin/issues/30036#issuecomment-2099367681)
Vouch, spoke to Secret Service this week. They are aware of the issue and a case is open for it. Your local office can assist.

💬 mzumsande commented on issue "Memory leak with `rest/block` REST endpoint and `getblock` RPC when verbosity >=2":
(https://github.com/bitcoin/bitcoin/issues/30052#issuecomment-2099375700)
> I could not reproduce an OOM, although I did observe resource usage increasing a little before eventually plateauing.

I see the same kind of behavior with Ubuntu with REST, the memory usage saturates after a few calls. So it looks like the same issue as in #25229 - at least for me.

💬 maciejsszmigiero 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#discussion_r1593153350)
Added a relevant constant.

💬 maciejsszmigiero 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-2099395788)
> If there's no drawbacks, why not go even larger?

I used 128 MiB as the new size for commonality with `MAX_BLOCKFILE_SIZE` already used by the block storage and because it gives me a nice low disk cache flush rate of about 1 flush / 2 seconds that no longer impacts the overall system performance.

But just to be sure, changed the patch to use `std::max()` around this `max_file_size` option so if at some point LevelDB decides to increase its default above 128 MiB we won't be lowering it acc
...

🤔 edilmedeiros requested changes to a pull request: "test: use assert_greater_than util"
(https://github.com/bitcoin/bitcoin/pull/30019#pullrequestreview-2044207210)
Concept ACK.

I have run all the tests that have changed, it seems ok.

Thanks for the contribution. But, since you are already refactoring this out, I believe it would be way better to not leave any more `assert` behind that could be changed to `assert_equal` or `assert_greater_than`. Otherwise, looks like we are not improving the code base as there will be more than one style of asserts in some files.

See the diffs in the comments to find some suggestions.

To find others, grep each
...

💬 edilmedeiros commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#discussion_r1593096616)
Change these to `assert_equal` is an easy gain here.

💬 edilmedeiros commented on pull request "test: use assert_greater_than util":
(https://github.com/bitcoin/bitcoin/pull/30019#discussion_r1593146698)
```diff
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
index cf48826e6a..415e4f1b95 100755
--- a/test/functional/feature_rbf.py
+++ b/test/functional/feature_rbf.py
@@ -15,6 +15,7 @@ from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
assert_equal,
assert_greater_than,
+ assert_greater_than_or_equal,
assert_raises_rpc_error,
)
from test_framework.wallet import MiniWallet
@@ -446,9 +447,9
...

💬 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.");
}
```
...