💬 zzzzasaa11 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-1864684692)
`master cricket property surface tooth million wash nominee priority wolf turtle notice`,`TXNLWRknggM7aq6W8z1YentbuqGV2tuDJg`
USDT is frozen. I am angry. It is impossible to give USDT to Tether. If you have the ability, take it.

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432869408)
I agree, see my comment in the description. Ultimately, I think `FundTransaction` should take a set of inputs and a set of outputs and return a `CreatedTransactionResult`. #25273 and this PR get us incrementally closer to that, but I don't want to fully tackle that in this PR as it's a bit more complicated than it seems, and the primary goal of this PR is to create the `CRecipients` objects in a standard way across all the RPC calls and pass them to `FundTransaction`

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1432881141)
Also a good catch, we should be using the `recipients` vector to get the size here.

📝 sr-gi opened a pull request: "test: adds outbound eviction functional tests, updated comment on ConsiderEviction"
(https://github.com/bitcoin/bitcoin/pull/29122)
## Motivation

While checking the outbound eviction code I realized a case was not considered within the comments, which in turn made me realize we had no functional tests for the outbound eviction case (when I went to check/add the test case).

This PR updated the aforementioned comment and adds functional tests to cover the outbound eviction logic, in addition to the unit tests covered by `src/test/denialofservice_tests.cpp`.

💬 sr-gi commented on pull request "test: adds outbound eviction functional tests, updated comment on ConsiderEviction":
(https://github.com/bitcoin/bitcoin/pull/29122#issuecomment-1864741013)
While the tests seem to run fine, I noticed a warning that appears every now and then:

```
TestFramework.p2p (WARNING): Connection lost to 0:0 due to [Errno 54] Connection reset by peer
```

I'm not sure if something unintended is happening under the hood. Disconnections are expected, but it's our node who is supposed to trigger them, so maybe I'm missing something.

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1864748743)
Fixed the `has_data` logic and replaced all references to `tx.vout` with `recipients` in `FundTransaction`, per feedback from @S3RK

💬 mzumsande commented on issue "Assertion failed: (data.size() > node.nSendOffset), function SocketSendData, file net.cpp, line 837":
(https://github.com/bitcoin/bitcoin/issues/27963#issuecomment-1864758276)
So it can happen both on M1 and M2.
Also, it's probably not due to something recently introduced in bitcoind, at least I can't see any changes between v25 and v25.1 that look like they could be related - so the bisecting suggested earlier probably won't lead to much either.

The fact that it happened on two unrelated instances almost at the same time is also weird. Maybe the operating system can get some kind of hiccup and then suddenly the `send` syscall returns incorrect values?

@mononau
...

💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1432731105)
In commit "wallet: Automatically upgrade a wallet to have global hd key" (ce5495a4b27fe9646c9933dc39f0cb4e8510eaff)

Would seem a little safer to write `if (!best_xpub || desc_time > best_time)` to work even if desc_time is not set.

💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1432741629)
In commit "wallet: Automatically upgrade a wallet to have global hd key" (ce5495a4b27fe9646c9933dc39f0cb4e8510eaff)

Can you write a comment explaining when this find is expected to fail (also the find below)?

Naively, I would think if this find fails, probably it means "best_xpub" is not really the best xpub. I would have expected these lookups to have already been done in one of the previous two loops and used to determine the best key candidate.

This is probably wrong, but either way,
...

🤔 ryanofsky reviewed a pull request: "wallet: Have the wallet store the key for automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/26728#pullrequestreview-1790921397)
Mostly reviewed 059d847810051b52d33b5b16711a031c3fd199a9 and it looks good, but a number of things were unclear to me and I left questions.

Another question I had was now that descriptor wallets are aware of the HD seed, does it make sense for getwalletinfo to start returning an hdseedid value? Does it make sense for sethdseed RPC to be supported? Assume the answer to these is no, but I'm not clear on why. I think it probably would make sense to say in the migratewallet documentation how thes
...

💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1432858600)
Not a comment for this PR specifically, but I think this `!Assume` code is hard to read and unnatural to parse, even though it does seem like a good way to handle bad assumptions at runtime. IMO, the code would be easier to read with a dedicated macro like:

```c++
ASSERT_ELSE(xpub.pubkey.IsValid(), return false);

ASSERT_ELSE(m_hd_crypted_key.empty(), return std::nullopt);

ASSERT_ELSE(pwallet->IsWalletFlagSet(WALLET_FLAG_GLOBAL_HD_KEY),
throw JSONRPCError(RPC_WALLET_ERROR, "Wallet
...

💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1432749103)
In commit "wallet: Automatically upgrade a wallet to have global hd key" (ce5495a4b27fe9646c9933dc39f0cb4e8510eaff)

Return value here is ignored. It would be good to add [[nodiscard]], (void) and comment saying why failure is ignored.

Instinctively, I'd say it even if makes sense to set the GLOBAL_HD_KEY flag when no HD key candidate is written, I don't think it makes sense to set it when there is an unexpected error trying to write it, or trying to find it.

This could be wrong, and may
...

💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1432889935)
In commit "wallet: Load activehdkey and its private keys" (2e840bcfc0308a461d210e0c7ed66a9bbc723b62)

It's not obvious what the relationship between `m_xpub`, `m_hd_key`, and `m_hd_crypted_key` is here. I think it would clearer to have a struct like:

```c++
struct HDKey {
CExtPubKey xpub;
//! Variant storing CKey if the wallet is unencrypted, otherwise the encrypted key.
std::variant<CKey, std::vector<unsigned char>> xpriv;
}
```

Then `CWallet` could just add:

```c++
...

💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1432799216)
In commit "walletdb: Read and write activehdkey record" (8bb7a8b06f1f0558bdb0bda696f5fb5748dacf7e)

This makes sense, but it sounds so fragile it makes me wonder why we want to store this unreliable value.

Can there be a comment (here or in LoadDescriptorWalletRecords) saying why it is necessary to store this? Or what the drawbacks would be of not storing it and just figuring out the active seed when the wallet is loaded, or when the gethdkey RPC is called?

💬 ryanofsky commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1432910673)
In commit "wallet: Load activehdkey and its private keys" (2e840bcfc0308a461d210e0c7ed66a9bbc723b62)

What does no deriviation will be performed mean? Would suggest clarifying, or maybe removing.

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

In commit "wallet: Load activehdkey and its private keys" (2e840bcfc0308a461d210e0c7ed66a9bbc723b62)

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

This is counterintuitive and I think it would be really helpful if one of the explanations of the ACTIVEHDKEY field (in the LoadDescriptorWa
...

💬 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