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

> Should we add some documentation about this quirk?

This is now described in the WalletBatch::WriteActiveHDKey comment, so marking this resolved.

💬 sr-gi commented on pull request "net: Attempts to connect to all resolved addresses on `addnode`":
(https://github.com/bitcoin/bitcoin/pull/28834#discussion_r1432947431)
Addressed in [9c1ab79](https://github.com/bitcoin/bitcoin/pull/28834/commits/9c1ab798b64d907e8aa96e0ed4d00469decab554)

📝 TheCharlatan opened a pull request: "build: Remove HAVE_CONSENSUS_LIB"
(https://github.com/bitcoin/bitcoin/pull/29123)
The `script/bitcoinconsensus` module defines the public interface for the `bitcoinconsensus` library. Even though this module is only required by the tests and the `bitcoinconsensus` library, it is currently compiled into the static internal `libbitcoin_consensus` library, and therefore used by a bunch of build targets that do not require it.

Since it is always part of the internal library, the `HAVE_CONSENSUS_LIB` define used by the tests is essentially
meaningless, since the module is alwa
...

💬 instagibbs commented on pull request "test: Add test case for speding bare multisig":
(https://github.com/bitcoin/bitcoin/pull/29120#discussion_r1432956992)
test looks fine, would be best if we cover all the allowed and border cases:
1) n-of-m, where `1 <= n <= 3` as well as `1 <= m <= 3` (do two loops)
2) too-large, i.e. n/m == 4
3) too-small(?) n/m==0

2/3 may require mining them into blocks directly if the outputs are non-standard to make

💬 TheCharlatan commented on pull request "build: Remove HAVE_CONSENSUS_LIB":
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1864811733)
cc @hebasto @theuni

💬 theuni commented on pull request "build: Remove HAVE_CONSENSUS_LIB":
(https://github.com/bitcoin/bitcoin/pull/29123#issuecomment-1864857056)
Concept ACK.

I think another way of looking at this is that `script/bitcoinconsensus.cpp` is moving out into its own convenience lib, it just happens to be a single file. We could imagine if the api were written in multiple translation units, we'd actually want to build that convenience lib. But the question is: absent that, is it worth the trouble?

Honestly I don't really care because it's kinda funky either way. Curious to see if anyone else has a preference.

💬 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-1864879265)
I was able to replicate the bug. I believe this is only an issue with 0.21 and 22.0 as the bug was fixed by #23304 which was first released in 23.0.

The issue occurs when inactive key chains are topped up. There was an off-by-one where loading would erroneously find the last index derived rather than the next index to be derived (last index + 1), and so it would end up re-deriving that last index, which I think is what causes the weird derivation path. Specifically, `DeriveNewChildKey` takes
...

👍 alfonsoromanz approved a pull request: "test: fix typo"
(https://github.com/bitcoin/bitcoin/pull/29121#pullrequestreview-1791382858)
ACK

💬 mononaut commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1864887312)
I've been running the two instances for several months in more or less the same conditions without any problems, and running them on that specific v25.1 commit for at least a few weeks.

It's possible there was more network activity happening on the machine than usual, but I couldn't say for sure.

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