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

✅ MarcoFalke closed an issue: "Berkley Database Errors"
(https://github.com/bitcoin/bitcoin/issues/28197)

💬 martinus commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#discussion_r1282836964)
yes, I'll clang-format the changes

💬 martinus commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#discussion_r1282837030)
Ah, actually I didn't think about temporaries at all... I changed it to be more in line with `Serialize` which uses `const Args&... args`, so `Args&... args` felt more appropriate.

I'll change it back to &&

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1282854035)
This issue has some helpful context: https://github.com/bitcoin/bitcoin/issues/12314#issuecomment-582600899

TLDR; at the time regtest was created, it was assumed that using a separate HRP wouldn't cost anything.

> If each of the four had a different one, I’d get it, but why does regtest have a different one than the other two testing networks?

This actually makes sense to me. Testnet and Signet are "global" networks, so everyone has to agree on the address format. Regtest is always used
...

💬 fanquake commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1663552242)
@hebasto did you open an issue to track this upstream? Can you link it here?

💬 martinus commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#issuecomment-1663556136)
Rebased to f054bd0 to clang-format the changes, and adds back the `&&` in `UnserializeMany`.