💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1593008473)
My thought was to avoid running another subprocess which will then make the same syscalls (excluding checking it's executable).

`--version` is less code on our side though, and more robust, so I will switch to that.

💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1593009480)
Yes, fixed in ffc691ac70b4a652fdf62e3a28876a5feb8d97c8

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1593012869)
Some of the option fields do currently hold references and it might be a good idea to move ownership of those referred values to the kernel Context eventually. I agree though that the comment should reflect the current code, not some future hypothetical.

💬 TheCharlatan commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#discussion_r1593014537)
A RAII wrapper for the ECC state sounds like a nice improvement.

⚠️ maflcko opened an issue: "ci: msan use-of-uninitialized-value when -jobs=1"
(https://github.com/bitcoin/bitcoin/issues/30057)
Nothing to do here, just leaving a note for reference.

When building the `ci_native_fuzz_msan` CI pod, and running inside of the pod a fuzz worker, it will report `use-of-uninitialized-value` inside libfuzzer.

```
FUZZ=parse_univalue /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz -max_total_time=1 # works
```

```
FUZZ=parse_univalue /ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz -max_total_time=1 -jobs=1 # fails
...

✅ maflcko closed an issue: "ci: msan use-of-uninitialized-value when -jobs=1"
(https://github.com/bitcoin/bitcoin/issues/30057)

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1592921496)
Something sus happened here: the tests seem to pass, but the message indicates encoding failure. Next is the beginning of the relevant log. It happens for all input vectors.

```
test/base58_tests.cpp:21: Entering test suite "base58_tests"
test/base58_tests.cpp:24: Entering test case "base58_EncodeBase58"
test/base58_tests.cpp:40: info: check '["",""]
Encoding failed for test #0: expected , got ' has passed
test/base58_tests.cpp:40: info: check '["61","2g"]
Encoding failed for test #1: e
...

🤔 edilmedeiros reviewed a pull request: "test: Add a few more corner cases to the base58 test suite"
(https://github.com/bitcoin/bitcoin/pull/30035#pullrequestreview-2043898870)
Concept ACK

Built and ran the unit tests.

The new messages seem to be the opposite of what the tests do, tough.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1592949918)
I like this change, but the message doesn't seem so good. I greped the repo but couldn't find a `maxRetLen` symbol, thus it sounds very confusing to me.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1592905348)
Since you are proposing random input vectors, what do you think about adding a log message with the generated test vector?

`BOOST_TEST_MESSAGE("Test input: '" << encoded << "'");`

looks good on my terminal. You probably can figure out a better message.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#discussion_r1592956103)
Same thing as above.

```
test/base58_tests.cpp:45: Entering test case "base58_DecodeBase58"
test/base58_tests.cpp:60: info: check '["",""]
Decoding failed for test #0: ' has passed
test/base58_tests.cpp:64: info: check '["",""]
Mismatch for test #0: expected , got ' has passed
test/base58_tests.cpp:60: info: check '["61","2g"]
Decoding failed for test #1: 2g' has passed
test/base58_tests.cpp:64: info: check '["61","2g"]
Mismatch for test #1: expected 61, got 61' has passed
test/base
...

💬 paplorinc commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#issuecomment-2099233436)
Thanks for the review @edilmedeiros, some of the existing BOOST_CHECK_MESSAGE messages are either in [indicative](https://github.com/bitcoin/bitcoin/blob/master/src/test/bloom_tests.cpp#L51) or [subjunctive](https://github.com/bitcoin/bitcoin/blob/master/src/test/bloom_tests.cpp#L31) grammatical moods (even in the same file, as you can see).
I'm fine with both, but if you think the subjunctive is more readable, I'll rephrase.

👍 hebasto approved a pull request: "Fix misleading signmessage error with segwit"
(https://github.com/bitcoin-core/gui/pull/819#pullrequestreview-2044111050)
ACK fb9f150759b22772dd48983a2be1ea397245e289.

💬 edilmedeiros commented on pull request "test: Add a few more corner cases to the base58 test suite":
(https://github.com/bitcoin/bitcoin/pull/30035#issuecomment-2099261379)
> Thanks for the review @edilmedeiros, some of the existing BOOST_CHECK_MESSAGE messages are either in [indicative](https://github.com/bitcoin/bitcoin/blob/master/src/test/bloom_tests.cpp#L51) or [subjunctive](https://github.com/bitcoin/bitcoin/blob/master/src/test/bloom_tests.cpp#L31) grammatical moods (even in the same file, as you can see). I'm fine with both, but if you think the subjunctive is more readable, I'll rephrase.

I have no personal preference about it, but this is not my point.
...

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