💬 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-2099135435)
FWIW re: #29662 I did not notice any difference in compaction time at startup on an SSD. It takes about 5 seconds to finish with `debug=leveldb` both on master and this branch.

💬 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-2099144448)
Concept ACK

Built and ran the unit tests.

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

💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592963190)
OK, the much simpler change I pushed in b6be80ca8c74852d4e2c1476527c4300be2125b8 has successfully [failed](https://cirrus-ci.com/task/6592292239179776) CI.

I am still a bit worried there may be other (like me) who have a `.venv` dir in their source dir, which will always fail the check. But got both options available now anyway.

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592988891)
Not sure about re-implementing `which`. Why not just call `mlc --version` or `mlc --help`, like in the shellcheck check?

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#discussion_r1592994881)
Is the conversion to string objects needed? According to the docs, `join` will make a String by itself, no? https://doc.rust-lang.org/std/slice/trait.Join.html#associatedtype.Output-1

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2099213678)
Rebased due to the conflict with merged bitcoin/bitcoin#29494.

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