💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282502389)
```suggestion
//! Substitute for std::monostate that doesn't depend on std::variant.
```
-----

(also, per the lint CI:)
```
src/util/result.h:30: Subsitute ==> Substitute
src/util/result.h:200: OT ==> TO, OF, OR, NOT
src/util/result.h:201: OT ==> TO, OF, OR, NOT
src/util/result.h:221: OT ==> TO, OF, OR, NOT
src/util/result.h:222: OT ==> TO, OF, OR, NOT
^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spel
...

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282503773)
<details><summary>Tidy CI iwyu suggestions</summary><p>

```diff
+#include <tinyformat.h>
+#include <util/translation.h3

+#include <algorithm>
+#include <memory>
+#include <ostream>
+#include <string>
+#include <utility>
```
</p></details>

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282507096)
6ddcc9be `GetErrors()` and `GetWarnings()` don't seem to have any test coverage yet. Note that there is also already a `GetWarnings()` in `src/warnings.{h.cpp}`, perhaps disambiguate.

🤔 furszy reviewed a pull request: "net_processing: re-allow fetching of genesis block via `getblockfrompeer`"
(https://github.com/bitcoin/bitcoin/pull/28205#pullrequestreview-1559984577)
I don't think that the capability of fetching the genesis block from the network is relevant enough to add an exception to the anti-DoS check. Nodes shouldn't waste resources asking for, nor responding to genesis block requests.
As you said, the genesis block is hardcoded on all nodes. Better to add the exception onto the `getblock()` RPC command to always retrieve it instead.

✅ 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)