👍 theStack approved a pull request: "rpc, test: `addnode` improv + add test coverage for invalid command"
(https://github.com/bitcoin/bitcoin/pull/26366#pullrequestreview-1559679246)
re-ACK f52cb02f700b58bca921a7aa24bfeee04760262b

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1282340187)
```
2023-08-02T19:32:23Z [sensitiverelay:debug]
Request for 5 new connections, incremented the number of connections to open from 90 to 95
```

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1282341818)
I meant, if IPv4 and Tor are the only reachable networks, that Tor connections would *only* be made for sensitive relay connections. That's why I tried `-onlynet=ipv4 -sensitiverelayowntx=1`

💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#issuecomment-1662864782)
Re `offtopic [[nodiscard]]`, yes. ISTM it's fine for an author not to take a review suggestion to use it, or for the two cases mentioned, to use it in code they write or touch. A bit like the usage of `const`, perhaps: it's a handy addition to C++, and if I have it wrong in a place, happy to drop.

> There are 3 commits ([5ba73cd](https://github.com/bitcoin/bitcoin/commit/5ba73cd0ee1e661ec4d041ac8ae7a60cfd31f0c0), [df48856](https://github.com/bitcoin/bitcoin/commit/df488563b280c63f5d74d5ac0fc
...

💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1662918852)
I've fixed at least one fuzzer crash. BDB verifies a couple of other things that RO was not.

I also changed the fuzzer to make sure that BDB fails to open the database if RO failed to open it, and vice versa.

There's still another crash that I have, but it's something where RO is being stricter than BDB.

💬 pinheadmz commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#discussion_r1282387190)
oh yeah I see this locally too (on master), even though `--cap-add SYS_PTRACE` is in the docker command and `USDT tracing` is set by configure, what's missing?

💬 0xB10C commented on pull request "ci: Move ASan USDT to persistent_worker":
(https://github.com/bitcoin/bitcoin/pull/28161#discussion_r1282404122)
> what's missing?

These are the checks to determine if we want to skip the USDT test: https://github.com/bitcoin/bitcoin/blob/2fa60f0b683cefd7956273986dafe3bde00c98fd/test/functional/interface_usdt_net.py#L87-L91

I haven't looked into this in detail. With which user are the test run in docker locally? Is `--cap-add SYS_PTRACE` enough?

💬 furszy 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_r1282405497)
Ok, I think that know how that could happen. It is not related to the `Init()` call.

The background sync process reads blocks directly from the chain index (calling `NextSyncBlock()`). So, what if a block gets connected while `ThreadSync()` is being executed but the event is not dispatched on time.. (before the background sync process finishes)

1) The block connection event is added to the validation queue.
2) The index background sync process reads the block from the chain and processes
...

💬 pinheadmz commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1282412077)
It's up to `332` now, after 45 min the wallet tried another rebroadcast. Also worth noting that so far none of these signet TXs are coming back to me, appearing on signet explorers, or reducing the mempool unbroadcastcount. I'll try to figure out why thats not working.

🤔 jonatack reviewed a pull request: "refactor: serialization simplifications"
(https://github.com/bitcoin/bitcoin/pull/28203#pullrequestreview-1559814631)
This looks like a very nice cleanup (and in a widely included and large file).

Have started running a node on this branch.

Initial approach ACK 513f4659b0a2ac4e2454ad2425b0fec8d7f64f04

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