✅ hebasto closed an issue: "Signmessage doesn't work with segwit addresses"
(https://github.com/bitcoin/bitcoin/issues/10542)

🚀 hebasto merged a pull request: "Fix misleading signmessage error with segwit"
(https://github.com/bitcoin-core/gui/pull/819)

💬 cbergqvist commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1593070661)
Without any additional changes. With `--enable-debug` and building with Clang.

I've retested on ecded92737c4173a30a43e612a21c6e1f24d1605, here is the combined log for `test/functional/p2p_seednode.py`:
[29605_timeout.combined_log.gz](https://github.com/bitcoin/bitcoin/files/15240761/29605_timeout.combined_log.gz)

It seems like I only have to bump 20 -> 23 seconds to prevent frequent timeouts. I've got IPv6 turned off but otherwise not much weird stuff going on AFAIK. Maybe something stand
...

💬 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_r1593114513)
Ah, now I get what you meant - I've changed the messages to shoulds, thanks for the hint!
Running it with `make && src/test/test_bitcoin --run_test=base58_tests --log_level=all -- -printtoconsole=1` now prints:
```
test/base58_tests.cpp:108: info: check 'Decoding `111111111111111J4kMuvqbHmtTVvULAvzkZCa3m2n9zHaYBL3KCE5y7phvY7737aYsgLjg5343Awj7jbBuYvioVwvkP4HvWsb3T6F8w3WEax4HkrTx8GXbZc1R1m4BWT48R8d2rU3557aNGpQ5pFyaFHYzTzDiNRTVmoYG7pns87AuFahyrTUNRufZe4gLf5Mb1JhWDCd438qHu8pBFWsVJrGkw` as `00000
...

💬 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_r1593116395)
Thanks, I've cherry picked this, in the current impl it's called `max_ret_len` - fixed

💬 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_r1593117357)
Fixed, thanks!
Added you as a Co-author, please check that the email is correct.

💬 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