💬 MarcoFalke commented on pull request "wallet: improve IBD sync time by skipping block scanning prior birth time":
(https://github.com/bitcoin/bitcoin/pull/27469#discussion_r1205026761)
> And in the wallet, we use the clock time to set the descriptors/keys creation time.

Ok, thanks for the background. However, I think the wallet uses mock time (NodeClock) for the times, so a much simpler fix would be just a single line patch to set the mock time to the genesis block time in the wallet bench that needs it?

💬 MarcoFalke commented on pull request "Use 'byte'/'bytes' for bech32(m) validation error message - 27727 follow up.":
(https://github.com/bitcoin/bitcoin/pull/27747#discussion_r1205031671)
```suggestion
std::string_view byte_str{data.size() == 1 ? "byte" : "bytes"};
```

nit: snake_case per dev notes and string_view to avoid a copy?

👍 MarcoFalke approved a pull request: "Use 'byte'/'bytes' for bech32(m) validation error message - 27727 follow up."
(https://github.com/bitcoin/bitcoin/pull/27747#pullrequestreview-1443136434)
lgtm

💬 martinus commented on pull request "bugfix: account for system-allocated memory in pool resource":
(https://github.com/bitcoin/bitcoin/pull/27748#discussion_r1205035430)
I agree, also with current memory accounting code its not possible to use the same memory resource for multiple containers. So it would be nice to add full accounting to the pool, and then remove the special handling of std::unordered_map with pool and just count the ressource's total usage.

💬 MarcoFalke commented on pull request "addrman: select addresses by network follow-up":
(https://github.com/bitcoin/bitcoin/pull/27745#discussion_r1205038328)
I don't think this is correct. Dereferencing an end-iterator is UB, so there is just no way to safely proceed in production, unless you add an early return (no idea if that is possible or even safe)

💬 russeree commented on pull request "rpc: Use 'byte'/'bytes' for bech32(m) validation error message":
(https://github.com/bitcoin/bitcoin/pull/27747#discussion_r1205046324)
Applied, committed, squashed, and pushed.

- Is there any need for `const` keyword with std::string_view since it is read only?

- Snake case has been remedied.

💬 MarcoFalke commented on pull request "kernel: Remove util/system from kernel library, interface_ui from validation.":
(https://github.com/bitcoin/bitcoin/pull/27636#discussion_r1205046358)
It's removed, no?

💬 MarcoFalke commented on pull request "rpc: Use 'byte'/'bytes' for bech32(m) validation error message":
(https://github.com/bitcoin/bitcoin/pull/27747#discussion_r1205048572)
```suggestion
std::string_view byte_str{data.size() == 1 ? "byte" : "bytes"};
```

💬 russeree commented on pull request "rpc: Use 'byte'/'bytes' for bech32(m) validation error message":
(https://github.com/bitcoin/bitcoin/pull/27747#discussion_r1205053788)
My apologies for not being attentive to detail. This has now been remedied.

⚠️ MarcoFalke opened an issue: "rpc_getblockfrompeer.py intermittent failure: assert_equal(pruneheight, 248); not(249 == 248)"
(https://github.com/bitcoin/bitcoin/issues/27749)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

https://cirrus-ci.com/task/4776911524593664?logs=ci#L3147

```
test 2023-05-24T12:16:53.212000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/tmp/cirrus-build-825335453/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, i
...

💬 MarcoFalke commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1562358177)
Looks like the last push bricked everything? Maybe revert to the previous version?

💬 MarcoFalke commented on pull request "refactor: Replace `std::optional<bilingual_str>` with `util::Result`":
(https://github.com/bitcoin/bitcoin/pull/25977#issuecomment-1562401646)
very nice ACK 8aa8f73adce72e1ae855b43413c1f65504423cb 🏏

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: very nice ACK 8aa8f73
...

💬 MarcoFalke commented on issue "Spurious (?) valgrind failure for p2p_compactblocks.py":
(https://github.com/bitcoin/bitcoin/issues/27741#issuecomment-1562417710)
Interesting. This should be a known bug. For now you can just use `clang` instead, see also the compilers that are known to be working with valgrind: (Your Ubuntu 23.04 should be identical to Debian Bookworm for the purposes here)

https://github.com/bitcoin/bitcoin/blob/a13f3746dccd9c4ec16d6bfe9b33ebd26e3238e1/contrib/valgrind.supp#L15-L17

💬 MarcoFalke commented on pull request "test: Disable legacy wallet for mempool_packages.py":
(https://github.com/bitcoin/bitcoin/pull/27735#issuecomment-1562433610)
Yeah, the test doesn't need the wallet, but it is providing mempool-package coverage for the `listunspent` wallet RPC. So maybe an alternative would be to move it out into a new test, idk?

💬 MarcoFalke commented on pull request "refactor: Replace `std::optional<bilingual_str>` with `util::Result`":
(https://github.com/bitcoin/bitcoin/pull/25977#discussion_r1205138680)
nit (if you retouch): Could add `[[nodiscard]]` (either as fixup or new commit), while touching all those header files?

💬 MarcoFalke commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#issuecomment-1562455232)
could rebase on current master to avoid CI upstream bug?

💬 MarcoFalke commented on pull request "script: add description for the functionality of each opcode":
(https://github.com/bitcoin/bitcoin/pull/27109#issuecomment-1562455454)
could rebase on current master to avoid CI upstream bug?

👍 TheCharlatan approved a pull request: "refactor: Replace `std::optional<bilingual_str>` with `util::Result`"
(https://github.com/bitcoin/bitcoin/pull/25977#pullrequestreview-1443309557)
ACK 8aa8f73adce72e1ae855b43413c1f65504423cb7

I think `[[nodiscard]` qualifiers for all the `util::Result` return types would be nice.

💬 MarcoFalke commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1562538942)
> I'm going to see if we can maybe port this over to our depends

I am not sure if compiling a compiler for a CI task is a worthwhile thing to maintain in this repo? Ideally we can use one from the Debian/Ubuntu distros, as they are used the most, so that it will be easy to bootstrap the test config outside the CI env. If the Debian/Ubuntu one isn't working, we could try into using a different distro temporarily? But imo compiling it from scratch should be for the last fallback and not a long
...

💬 MarcoFalke commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#discussion_r1205214490)
Looks like memory usage isn't reported in https://cirrus-ci.com/task/4543816401682432 ?