💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282131242)
comment block seems extraneous if there's logging for each test

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282258402)
can we assert that peer_spy hasn't received the INV, just to be sure?

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282259677)
I don't get this case. Why wouldn't you request a "fake" parent if you don't know it's fake already?

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282268679)
"The node should not request a parent if it has an in-flight txrequest" ? Seems like parents are being reqiested. Maybe a typo or I can't tell what scenario it's covering.

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282261573)
does this wallet never use 0-conf change? would be nice to assert to make it clear the lack of utxo connection

📝 hebasto opened a pull request: "qa: Close SQLite connection properly"
(https://github.com/bitcoin/bitcoin/pull/28204)
This PR is a follow-up for https://github.com/bitcoin/bitcoin/pull/26462 that introduced a bug on Windows:
```
>test\functional\wallet_descriptor.py
...
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process:
...
```

From `sqlite3` Python module [docs](https://docs.python.org/3/library/sqlite3.html#how-to-use-the-connection-context-manager):
> `Connection` object used as context manager only commits or rollbacks transactions, so th
...

💬 hebasto commented on pull request "wallet: fix crash on loading descriptor wallet containing legacy key type entries":
(https://github.com/bitcoin/bitcoin/pull/26462#issuecomment-1662749647)
The test is broken on Windows:
```
>test\functional\wallet_descriptor.py
...
PermissionError: [WinError 32] The process cannot access the file because it is being used by another process:
...
```

Fixed in https://github.com/bitcoin/bitcoin/pull/28204.

💬 hebasto commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#discussion_r1282298267)
```suggestion
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
```

💬 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#issuecomment-1662828515)
Updated e025bb3f8d229969fdfe16e3c20191a5382c0443 -> c553d9f0b0f796c30124fddb920b5e8b5b32de11 ([`pr/indexy.41`](https://github.com/ryanofsky/bitcoin/commits/pr/indexy.41) -> [`pr/indexy.42`](https://github.com/ryanofsky/bitcoin/commits/pr/indexy.42), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/indexy.41..pr/indexy.42)) to avoid race condition pointed out by furszy in an intermediate commit: https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1282110542 which could potentially
...

👍 theStack approved a pull request: "qa: Close SQLite connection properly"
(https://github.com/bitcoin/bitcoin/pull/28204#pullrequestreview-1559672559)
utACK 703b758e187492d4752270cd9922eaf0af20e2d0

Thanks for fixing! Can't reproduce the error locally as I don't have a Windows machine available, but the change looks correct to me and is in line with the Python documentation (https://docs.python.org/3/library/sqlite3.html#sqlite3-connection-context-manager).

💬 stickies-v commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#discussion_r1282316651)
Alright yeah I was a bit too quick with my initial suggestion, the current implementation makes sense from a (de)alloc perspective, thanks for pointing that both.

> Let me know if you think passing them arguments here is more confusing.

I still don't really like it, but I agree that taking these 2 arguments is the most natural interface for `WriteImpl`. The only reason we have this weirdness is just because `Write` wants to do some allocation optimization. So I'm okay keeping it like this.
...

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