💬 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
(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
...
(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.
(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,
...
(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
...
(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
...
(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
...
(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++
...
(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?
(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.
(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
...
(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.
(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)
(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
...
(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
(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
(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.
(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
...
(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
(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.
(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.