💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282251680)
took me a second, just assert it's not in mempool for those quickly reading

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282234657)
```suggestion
# The parent should be requested since the unstripped wtxid would differ. Delayed because it's by txid and this is not a preferred relay peer.
```

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282208201)
Stick the constant inside `cleanup` and move the comment for `cleanup`, since it explains what the whole thing is doing.

💬 instagibbs commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1282243358)
can't we just reconsider it again?

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