🤔 mzumsande reviewed a pull request: "net: Attempts to connect to all resolved addresses on `addnode`"
(https://github.com/bitcoin/bitcoin/pull/28834#pullrequestreview-1791425322)
Would be good to adjust the title / commit message to be more general:
The solution doesn't specifically address `addnode` but changes `OpenNetworkConnection` / `ConnectNode` and therefore all callers that use it with `pszDest` set (`-connect`, `-seednode`, test-only `-addconnection`) are affected by the change.

👍 hernanmarino approved a pull request: "getrawtransaction implementation"
(https://github.com/bitcoin-core/gui/pull/777#pullrequestreview-1791462649)
Concept ACK.

💬 0xB10C commented on pull request "doc: Add multiprocess design doc":
(https://github.com/bitcoin/bitcoin/pull/28978#discussion_r1433109875)
```suggestion
</td></tr></table>
```

I think you meant to close/end the table here.

👋 LarryRuane's pull request is ready for review: "test: test_bitcoin: allow -testdatadir=<datadir>"
(https://github.com/bitcoin/bitcoin/pull/26564)

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1433153712)
Thanks for the suggestion; I decided to name the extra path component `test_temp` instead of `tests`, because if the user thought it was okay to specify their home directory, they may have a directory there called `tests` with some source code. This name isn't likely to conflict, and the `temp` part of the name indicates that it's a temporary directory (unlike `signet`, `regtest`, etc., which make sense to persist).

💬 c0deright commented on issue "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect":
(https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1865108566)
Thanks for your thorough explanation of this bug!

📝 achow101 opened a pull request: "wallet: Automatically repair corrupted metadata with doubled derivation path"
(https://github.com/bitcoin/bitcoin/pull/29124)
A bug in 0.21.x and 22.x resulted in some wallets storing invalid derivation paths that are the concatenation of two derivation paths. This corruption follows a rigid pattern and can be easily detected and repaired.

Fixes #29109

💬 achow101 commented on issue "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect":
(https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1865119581)
#29124 should automatically fix the bad metadata. Can you try running that?

💬 LarryRuane commented on pull request "test: test_bitcoin: allow -testdatadir=<datadir>":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1865121551)
Force pushed the changes described above (https://github.com/bitcoin/bitcoin/pull/26564#discussion_r1428581458( but I don't understand why CI is failing. All the failures are that `LockDirectory()` is not found, but it's declared in `util/fs_helpers.h` and that file is included. It builds for me. @furszy, can you try building this branch? Thanks.

💬 sr-gi commented on pull request "net: attempts to connect to all resolved addresses when connecting to a node":
(https://github.com/bitcoin/bitcoin/pull/28834#issuecomment-1865127780)
> Would be good to adjust the title / commit message to be more general: The solution doesn't specifically address `addnode` but changes `OpenNetworkConnection` / `ConnectNode` and therefore all callers that use it with `pszDest` set (`-connect`, `-seednode`, test-only `-addconnection`) are affected by the change.

Updated both commit message/title and PR title/desciption

🤔 ryanofsky reviewed a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1791595622)
re: https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419188377

> Activeness of descriptors does not necessarily correlate to the hdkey. The user could have imported new active descriptors with different keys and the hdkey would not change.

Looking at related PRs which will build on this one, this approach seems increasingly kludgy to me.

It seems to me wallet usability will be worse and behavior is more confusing if:

- the active hd key (seed)
- the active set of descripto
...

💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1433143380)
re: https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1419188377

> Activeness of descriptors does not necessarily correlate to the hdkey. The user could have imported new active descriptors with different keys and the hdkey would not change.

Looking at related PRs which will build on this one, this approach seems increasingly kludgy to me.

It seems to me wallet usability will be worse and behavior is more confusing if:

- the active hd key (seed)
- the active set of descripto
...

💬 eragmus commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1865149097)
> Even if 100% of bitcoin nodes adopt this code, inscriptions of a different form would still persist in the mempool, provided demand for inscriptions also persists.

@conduition No need to switch to a different inscription form, assuming you mean 100% of non-mining nodes adopt it. The mining nodes (miners) are financially incentivized to accept the inscriptions, so they will not adopt such code, so inscriptions txs will be sent directly to miners out of band via dark/private mempools.

This the
...

💬 sipa commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1865155764)
utACK a30a1d9318d08ddd1007d7fe7c19e4175faa8b64

Conceptually, I believe this makes sense both now and eventually:
* Right now, anyone using `-v2transport` is opting into the BIP324 experiment. If this causes problems for them (e.g. because somehow addnode with default v2 causes issues), they always have the ability to just not do that.
* Eventually, having V2 default for addnode/connect makes sense. There is a significant UX advantage to not needing to specify, and BIP324 qualities are most
...

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1865160784)
> I think it would be better not to introduce the concept of an "active hd key" at all, and instead follow an approach more like what apoelstra implemented in #27351 where a hd seed is just extra information associated with a descriptor, and different descriptors can have different seeds.

This is essentially what we do today, albeit with hd keys rather than hd seeds.

> I think it is useful to have a `gethdkey` method similar to the one added here that would return the hd key associated wit
...

💬 sipa commented on pull request "wallet: fix key parsing check for miniscript expressions":
(https://github.com/bitcoin/bitcoin/pull/29027#issuecomment-1865161571)
utACK e1281f1bbd884f15d40053c9bc24794d0ce9a58a

💬 c0deright commented on issue "Old wallet.dat: Error reading wallet database: keymeta found with unexpected path / All keys read correctly, but transaction data or address metadata may be missing or incorrect":
(https://github.com/bitcoin/bitcoin/issues/29109#issuecomment-1865189611)
Looks good:

```
2023-12-20T21:34:16Z init message: Loading wallet…
2023-12-20T21:34:16Z BerkeleyEnvironment::Open: LogDir=/home/foobar/.bitcoin/database ErrorFile=/home/foobar/.bitcoin/db.log
2023-12-20T21:34:16Z [default wallet] Wallet file version = 10500, last client version = 220000
2023-12-20T21:34:16Z [default wallet] Repairing derivation path for 0221......850[default wallet] Repairing derivation path for 035c.....d0b[default wallet] Legacy Wallet Keys: 657 plaintext, 0 encrypted,
...

👍 TheCharlatan approved a pull request: "bench: add readblock benchmark"
(https://github.com/bitcoin/bitcoin/pull/26684#pullrequestreview-1791716209)
ACK 1c4b9cbe906507295d8b7d52855de1441ad411dd

💬 TheCharlatan commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1433214881)
Nit: This could be `BlockManager&` already.

💬 TheCharlatan commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1433213157)
NIt: Could make this IWYU correct (see the logs from the tidy CI job).