💬 achow101 commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1282220244)
Done

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1282227280)
Ah, figured it out! Will push some new entries to the qa-assets PR that should reproduce the crash.

💬 jonatack commented on pull request "rpc, util: use string_view for passing string literals to Parse{Hash,Hex}":
(https://github.com/bitcoin/bitcoin/pull/28172#issuecomment-1662703920)
Updated to pass string literals to the `Parse{Hash,Hex}` functions by value as `string_view` rather than `std::string`. Thank you for the helpful feedback!

<details><summary>code demonstrating the perf improvement</summary><p>

```cpp
// passing-string-literals.cpp
//
// $ g++ -std=c++17 passing-string-literals.cpp && ./a.out
// StringByValue("string literal") took 1426 cycles
// StringByConstRef("string literal") took 1412 cycles
// StringViewByValue("string literal") took 612 cycles
...

💬 Sjors commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1662720996)
I tried adding `if (bdb_ro_err) return;` above `#ifdef USE_BDB` (although that does reduce coverage). I also added in the second catch:

```cpp
// We are not interested in fuzzing the original BDB implementation
if (std::string(e.what()) == "BerkeleyDatabase: Error 22, can't open database fuzzed_wallet.dat") return;
```

That helps a bit, but still leads to a "SEGV on unknown address 0x000000000000" in `DumpWallet` very quickly. I haven't looked at what the fuzzer came up with. Perhaps it
...

📝 martinus opened a pull request: "refactor: serialization simplifications"
(https://github.com/bitcoin/bitcoin/pull/28203)
This simplifies the serialization code a bit and should also make it a bit faster.

* use fold expressions instead of recursive calls. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations.

* use `if constexpr` instead of unnecessarily creating a temporary object only to call the right overload. This is used for `std::vector` and `prevector` serialization.

🤔 instagibbs reviewed a pull request: "test: tx orphan handling"
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1559366940)
moar coverage good

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