💬 jonatack commented on pull request "refactor: serialization simplifications":
(https://github.com/bitcoin/bitcoin/pull/28203#discussion_r1282401045)
0637f53afbd9f1357ce92270f4d13a5f891d1657 question: you've followed the existing code, but I wonder if there is any reason why `SerializeMany` isn't `inline` like the neighboring methods.

💬 jonatack commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1662963276)
Concept ACK

💬 ryanofsky commented on pull request "indexes: Stop using node internal types and locking cs_main, improve sync logic":
(https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282421606)
That's accurate, but I don't think ThreadSync needs to even do anything for this to happen, and no new blocks need to be connected after ThreadSync starts.

If `-reindex-chainstate` indexing completes, and ThreadSync immediately starts and exits because it detects that the index best block is equal to the chain tip, and no new blocks are connected, there could still be stale block connected notifications in the queue which will cause the index to rewind and add the blocks again. The commit
"
...

💬 jonatack commented on pull request "script: throw disabled err for op_ver and its variants":
(https://github.com/bitcoin/bitcoin/pull/28169#issuecomment-1662971265)
ACK a4dcdfde2e3d831a562d996fb2c21aa549764c44

💬 Junrichony16 commented on issue "migratewallet crashes (wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.)":
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1663030746)
2023-07-20T16:40:23Z [main] [init/common.cpp:153] [LogPackageVersion] Bitcoin Core version v25.99.0-d23fda05842b (release build)


2023-07-20T16:40:56Z [qt-rpcconsole] [wallet/wallet.h:895] [WalletLogPrintf] [default wallet] Migrating wallet storage database from BerkeleyDB to SQLite.



2023-07-20T17:10:25Z [scheduler] [net.cpp:1523] [DumpAddresses] [net] Flushed 0 addresses to peers.dat 56ms


bitcoin-qt: wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacySc
...

📝 theStack opened a pull request: "net_processing: re-allow fetching of genesis block via `getblockfrompeer`"
(https://github.com/bitcoin/bitcoin/pull/28205)
Since commit ed6cddd98e32263fc116a4380af6d66da20da990 (PR #25717) incoming BLOCK messages have to pass an anti-DoS check in order to be accepted. Passing this check is currently only possible if there's a previous block available, which is obviously not the case for the genesis block, so it is denied:

`ERROR: ProcessNewBlock: AcceptBlock FAILED (too-little-chainwork)`

Fix that by adding the special case for the genesis block, so fetching it via `getblockfrompeer` on pruned nodes (which
...

🤔 jonatack reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1559945927)
ACK 08f5febfc571220043436bbec96a326beebdee22

Some non-blocking comments follow.

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282499219)
584e3fa Not sure, but these `if (!result) return result;` idioms (here and lines 228-229 below) seem "odd" enough that an explanatory comment might be helpful.

💬 jonatack commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1282487687)
Here and line 192 below: "earning"?

💬 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