✅ kallewoof closed a pull request: "BIP-322 basic support"
(https://github.com/bitcoin/bitcoin/pull/24058)

💬 MarcoFalke commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#discussion_r1282673617)
> Is `--cap-add SYS_PTRACE` enough?

No it is not. You can see the second commit of this pull request to see what is needed: fa474397b5d4235efdfc5a5ddce2d637234548a7

💬 MarcoFalke commented on pull request "qa: Close SQLite connection properly":
(https://github.com/bitcoin/bitcoin/pull/28204#issuecomment-1663331414)
lgtm ACK 703b758e187492d4752270cd9922eaf0af20e2d0

💬 MarcoFalke commented on pull request "qa: Close SQLite connection properly":
(https://github.com/bitcoin/bitcoin/pull/28204#issuecomment-1663333239)
Interesting that this isn't caught on Windows in Cirrus CI? https://cirrus-ci.com/task/5010562627665920?logs=functional_tests#L210

💬 martinus commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#discussion_r1282681586)
inline as a way to nudge the compiler into optimizing things is nowadays pretty useless, and as far as I know compilers simply ignore it anyways and do their own thing. Inlining only makes sense to mark implementations in a header so you won't get duplicate symbols at link time, but for templates that's not necessary because they are inline by default. So I'd say its best to remove all `inline` from any template function as it won't make any difference.

👍 MarcoFalke approved a pull request: "rpc, util: use string_view for passing string literals to Parse{Hash,Hex}"
(https://github.com/bitcoin/bitcoin/pull/28172#pullrequestreview-1560301083)
string_view is dangerous, because it can lead to use-after-free, but here it seems fine. lgtm.

I think you can remove the 4 links to random blog posts, because everyone is able to type `std::string_view` into a search engine, if they want to. Also, I don't think the benchmark is useful in this context and can be removed. If we cared about an RPC call taking a few femtoseconds less, there are much lower hanging fruits to pick. A benchmark would be useful if this showed an end-to-end improvemen
...

💬 MarcoFalke commented on pull request "rpc, util: use string_view for passing string literals to Parse{Hash,Hex}":
(https://github.com/bitcoin/bitcoin/pull/28172#discussion_r1282693452)
```suggestion
uint256 ParseHashV(const UniValue& v, std::string_view name)
```

nit, while touching all lines where this is used

💬 MarcoFalke commented on pull request "rpc, util: use string_view for passing string literals to Parse{Hash,Hex}":
(https://github.com/bitcoin/bitcoin/pull/28172#discussion_r1282693713)
same

💬 MarcoFalke commented on pull request "wallet: Allow `bumpfee` for txs that don't signal BIP-125":
(https://github.com/bitcoin/bitcoin/pull/26454#issuecomment-1663392857)
Closing for now due to prolonged inactivity and red CI since this pull was opened. Please leave a comment in this thread, so that it can be reopened, once and if you start working on this again.

✅ MarcoFalke closed a pull request: "wallet: Allow `bumpfee` for txs that don't signal BIP-125"
(https://github.com/bitcoin/bitcoin/pull/26454)

💬 hebasto commented on pull request "qa: Close SQLite connection properly":
(https://github.com/bitcoin/bitcoin/pull/28204#issuecomment-1663416385)
> Interesting that this isn't caught on Windows in Cirrus CI? [cirrus-ci.com/task/5010562627665920?logs=functional_tests#L210](https://cirrus-ci.com/task/5010562627665920?logs=functional_tests#L210)

The reason of that is the `--nocleanup` option: https://github.com/bitcoin/bitcoin/blob/2fa60f0b683cefd7956273986dafe3bde00c98fd/.cirrus.yml#L188

👍 hebasto approved a pull request: "refactor: Remove unused includes from wallet.cpp"
(https://github.com/bitcoin/bitcoin/pull/28200#pullrequestreview-1560401063)
ACK fa8940852f88f74cb0d4975e05ec32fd3594961c

💬 hebasto commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#discussion_r1282769866)
It seems `IWYU pragma: export` causes:
```
txmempool.h should add these lines:
...
#include "kernel/mempool_limits.h"
#include "kernel/mempool_options.h"
#include "kernel/mempool_removal_reason.h" // for MemPoolRemovalReason (ptr only)
...
```

Perhaps, a better solution is to use `contrib/devtools/iwyu/bitcoin.core.imp`.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1282771837)
"we disconnect incoming connections that match an already existing nonce" _but only for connections with incomplete handshake_. So the spy has to try to connect back to the node copying the nonce before they send ther `VERACK`, ie before they know this is a sensitive relay connection.

👍 MarcoFalke approved a pull request: "refactor: serialization simplifications"
(https://github.com/bitcoin/bitcoin/pull/28203#pullrequestreview-1560401108)
very nice, ACK 513f4659b0a2ac4e2454ad2425b0fec8d7f64f04 🙊

<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 513f
...

💬 MarcoFalke commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#discussion_r1282763406)
48adef0e41700e4b5d0072c71ffaa2387cdeccbb: Any reason to change this to disallow temporaries (`&&`)?

Also this commit can be squashed in the previous one?

Also can remove `inline`. (`template` enforces `inline` already)

💬 MarcoFalke commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#discussion_r1282760383)
0637f53afbd9f1357ce92270f4d13a5f891d1657: forgot clang-format on touched code?

💬 MarcoFalke commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#discussion_r1282771203)
review note in 10105d97f412cc54ba4031c56538f7e0c611251f: A simple `is_same_v` work, because I presume `T` can never include `const` in a prevector, similar to how `std::vector<const int>` does not exist.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1282776719)
No, I did not consider that.

💬 MarcoFalke commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#discussion_r1282779911)
Yes, I am aware, but I think this bug should be reported to IWYU, or is it not a bug?