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

💬 fanquake commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#issuecomment-1562543301)
> I am not sure if compiling a compiler for a CI task is a worthwhile thing to maintain in this repo?

This point is in regards to the libc++ flag usage. Not compiling a compiler for anything else.

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

Is that a CIrrus bug? You should be able to see an example memory usage here: https://cirrus-ci.com/task/4632561431871488.

🚀 fanquake merged a pull request: "p2p: Unconditionally return when compact block status == READ_STATUS_FAILED"
(https://github.com/bitcoin/bitcoin/pull/27743)

💬 MarcoFalke commented on pull request "ci: compile Clang and compiler-rt in msan jobs":
(https://github.com/bitcoin/bitcoin/pull/27737#discussion_r1205226114)
ah ok, so it is using 12 GB?

Looks like it is only passing CI because Cirrus seems to be ignoring the 2CPU/8GB limit and just uses 4/15 unconditionally for ci image builds?

💬 fanquake commented on pull request "test: Disable legacy wallet for mempool_packages.py":
(https://github.com/bitcoin/bitcoin/pull/27735#issuecomment-1562569950)
cc @glozow

💬 fanquake commented on issue "rpc_getblockfrompeer.py intermittent failure: assert_equal(pruneheight, 248); not(249 == 248)":
(https://github.com/bitcoin/bitcoin/issues/27749#issuecomment-1562574435)
@Sjors