💬 furszy commented on issue "migratewallet crashes (wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.)":
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1629096788)
Could you provide the `getaddressinfo mqRzyzrW7DPGT1KZN2mLuyGPVUugpfEVPW` output please.
And also, do you have steps to reproduce it? how did you import that address?
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1629096788)
Could you provide the `getaddressinfo mqRzyzrW7DPGT1KZN2mLuyGPVUugpfEVPW` output please.
And also, do you have steps to reproduce it? how did you import that address?
💬 MarcoFalke commented on issue "migratewallet crashes (wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.)":
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1629109069)
No idea where it is from. Apparently it is from a "test_2" :sweat_smile:
```
{
"address": "mqRzyzrW7DPGT1KZN2mLuyGPVUugpfEVPW",
"scriptPubKey": "76a9146cc16c24e249bab4384d4a759fe53ab23227de1988ac",
"ismine": false,
"solvable": false,
"iswatchonly": true,
"isscript": false,
"iswitness": false,
"ischange": false,
"timestamp": 0,
"labels": [
"test_2"
]
}
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1629109069)
No idea where it is from. Apparently it is from a "test_2" :sweat_smile:
```
{
"address": "mqRzyzrW7DPGT1KZN2mLuyGPVUugpfEVPW",
"scriptPubKey": "76a9146cc16c24e249bab4384d4a759fe53ab23227de1988ac",
"ismine": false,
"solvable": false,
"iswatchonly": true,
"isscript": false,
"iswitness": false,
"ischange": false,
"timestamp": 0,
"labels": [
"test_2"
]
}
👍 ryanofsky approved a pull request: "test: Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1522239261)
Code review ACK ca91c244ef1ba7eac6643d66a5fc56d3a2a8b550. The new tests seem useful and are very easy to read, and I think switching CI to run as non-root is a nice change that should make CI environment more useful and realistic.
I do think the instinct to skip tests is a mistake, though. Usually better to just let tests run in any environment and check whatever the expected behavior is in that environment, unless the behavior is really unstable or nondeterministic. In this case, rather than
...
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1522239261)
Code review ACK ca91c244ef1ba7eac6643d66a5fc56d3a2a8b550. The new tests seem useful and are very easy to read, and I think switching CI to run as non-root is a nice change that should make CI environment more useful and realistic.
I do think the instinct to skip tests is a mistake, though. Usually better to just let tests run in any environment and check whatever the expected behavior is in that environment, unless the behavior is really unstable or nondeterministic. In this case, rather than
...
💬 ryanofsky commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1258364600)
In commit "init: start indexes sync earlier" (ed4462cc78afd2065bbf5bd79728852b65b9ad7f)
Nice test. Blockman API is really confusing but the comments here make everything very clear, and having the test should make improvements easier to understand, and help ensure things don't get broken unintentionally.
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1258364600)
In commit "init: start indexes sync earlier" (ed4462cc78afd2065bbf5bd79728852b65b9ad7f)
Nice test. Blockman API is really confusing but the comments here make everything very clear, and having the test should make improvements easier to understand, and help ensure things don't get broken unintentionally.
⚠️ vasild opened an issue: "Auto detect IPv6 connectivity"
(https://github.com/bitcoin/bitcoin/issues/28061)
### Please describe the feature you'd like to see added.
Auto detect if the host has connectivity to the IPv6 network and only attempt to make automatic outbound ones if it has.
### Is your feature related to a problem, if so please describe it.
IPv6 is set to reachable by default. If it is not in practice then some outbound connection attempts will be made in vain, trying to connect to IPv6 hosts.
### Describe the solution you'd like
If the kernel does not have IPv6 support compiled in the
...
(https://github.com/bitcoin/bitcoin/issues/28061)
### Please describe the feature you'd like to see added.
Auto detect if the host has connectivity to the IPv6 network and only attempt to make automatic outbound ones if it has.
### Is your feature related to a problem, if so please describe it.
IPv6 is set to reachable by default. If it is not in practice then some outbound connection attempts will be made in vain, trying to connect to IPv6 hosts.
### Describe the solution you'd like
If the kernel does not have IPv6 support compiled in the
...
💬 fanquake commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1629153391)
> This will still fail outside the CI.
Yes, but outside the CI, the only route to solving that would be with more (achitecture specific) suppressions? If we don't control anything else, we can't make any guarantees about anything passing or not.
At least in this PR, it no-longer fails inside the CI, and that's difference between the CI working, and not, on aarch64.
Happy to followup with further improvements to add more architecture specific supressions or similar, for use outside the C
...
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1629153391)
> This will still fail outside the CI.
Yes, but outside the CI, the only route to solving that would be with more (achitecture specific) suppressions? If we don't control anything else, we can't make any guarantees about anything passing or not.
At least in this PR, it no-longer fails inside the CI, and that's difference between the CI working, and not, on aarch64.
Happy to followup with further improvements to add more architecture specific supressions or similar, for use outside the C
...
💬 vasild commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1629155080)
> a majority of my network specific connection attempts were to IPV6 addresses, even though I don't have an IPV6 address and can't make connections to it
Yeah. That is also an issue on `master`, without this PR. Maybe it deserves a separate PR. Opened https://github.com/bitcoin/bitcoin/issues/28061 to track that.
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1629155080)
> a majority of my network specific connection attempts were to IPV6 addresses, even though I don't have an IPV6 address and can't make connections to it
Yeah. That is also an issue on `master`, without this PR. Maybe it deserves a separate PR. Opened https://github.com/bitcoin/bitcoin/issues/28061 to track that.
🤔 ishaanam reviewed a pull request: "wallet: do not backdate locktime if it may lead to fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/26902#pullrequestreview-1522310111)
This is a good start, but I think that these transactions would be harder to fingerprint if the following modifications were made:
1. For RBF transactions: the `nLockTime` is set to the same value as the transaction being replaced.
2. For spending unconfirmed utxos: Use the same behavior as before, but make sure that the `nLockTIme` value is greater than or equal to the highest `nLockTIme` of the inputs.
For convenience I've implemented this here: https://github.com/ishaanam/bitcoin/commi
...
(https://github.com/bitcoin/bitcoin/pull/26902#pullrequestreview-1522310111)
This is a good start, but I think that these transactions would be harder to fingerprint if the following modifications were made:
1. For RBF transactions: the `nLockTime` is set to the same value as the transaction being replaced.
2. For spending unconfirmed utxos: Use the same behavior as before, but make sure that the `nLockTIme` value is greater than or equal to the highest `nLockTIme` of the inputs.
For convenience I've implemented this here: https://github.com/ishaanam/bitcoin/commi
...
💬 furszy commented on issue "migratewallet crashes (wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.)":
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1629158947)
Ok, found the `Unrecognized descriptor found` error cause. You might have imported a hex script and the migration process translated into a `sh(addr(mqRzyzrW7DPGT1KZN2mLuyGPVUugpfEVPW))#0ezg4j29` descriptor. Which isn't a valid descriptor because the `addr()` func can only appear at the top level.
The assertion cause is still unknown but hey, one bug less at least :).
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1629158947)
Ok, found the `Unrecognized descriptor found` error cause. You might have imported a hex script and the migration process translated into a `sh(addr(mqRzyzrW7DPGT1KZN2mLuyGPVUugpfEVPW))#0ezg4j29` descriptor. Which isn't a valid descriptor because the `addr()` func can only appear at the top level.
The assertion cause is still unknown but hey, one bug less at least :).
🚀 fanquake merged a pull request: "test: make assumeUTXO test capture the expected fatal error"
(https://github.com/bitcoin/bitcoin/pull/28050)
(https://github.com/bitcoin/bitcoin/pull/28050)
💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629211279)
Hey @MarcoFalke should I revert the last commit? All checks except the lint test are passing.
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629211279)
Hey @MarcoFalke should I revert the last commit? All checks except the lint test are passing.
👍 TheCharlatan approved a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1522386920)
re-ACK ca91c244ef1ba7eac6643d66a5fc56d3a2a8b550
(https://github.com/bitcoin/bitcoin/pull/27607#pullrequestreview-1522386920)
re-ACK ca91c244ef1ba7eac6643d66a5fc56d3a2a8b550
💬 sipa commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629226202)
Don't revert the commit; just remove the stray file and squash.
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629226202)
Don't revert the commit; just remove the stray file and squash.
💬 vasild commented on issue "p2p sends local-scope addresses when IPv4 is behind NAT":
(https://github.com/bitcoin/bitcoin/issues/8616#issuecomment-1629232150)
To summarize, there are 3 issues here:
1. The problem from OP of sending local addresses to peers was nonexistent. That was never done.
2. Sending our addr in the `VERSION` message, fixed by https://github.com/bitcoin/bitcoin/pull/8740
3. Sending our addr via `ADDR` (or `ADDRv2`) message. I need to look at this more carefully, but a brief look tells me that maybe the problem exists. I mean sending our IPv4 address to a Tor peer or sending our Tor address to an IPv4 peer. At least `CNetAddr:
...
(https://github.com/bitcoin/bitcoin/issues/8616#issuecomment-1629232150)
To summarize, there are 3 issues here:
1. The problem from OP of sending local addresses to peers was nonexistent. That was never done.
2. Sending our addr in the `VERSION` message, fixed by https://github.com/bitcoin/bitcoin/pull/8740
3. Sending our addr via `ADDR` (or `ADDRv2`) message. I need to look at this more carefully, but a brief look tells me that maybe the problem exists. I mean sending our IPv4 address to a Tor peer or sending our Tor address to an IPv4 peer. At least `CNetAddr:
...
🚀 ryanofsky merged a pull request: "index: make startup more efficient"
(https://github.com/bitcoin/bitcoin/pull/27607)
(https://github.com/bitcoin/bitcoin/pull/27607)
💬 pinheadmz commented on pull request "wallet: Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1258472175)
> I'm not sure if or when legacy wallet support is expected to be phased out, but if isactive is only different from ismine in legacy wallets, is it worth adding a field (and all the new methods), versus just updating the ismine field documentation in the help for descriptor wallets? (When I'm double-checking an address with getaddressinfo before using it to receive, ismine is a field I'm already checking.)
@jonatack after diving back in to this issue I realized that `ismine != isactive` for
...
(https://github.com/bitcoin/bitcoin/pull/27216#discussion_r1258472175)
> I'm not sure if or when legacy wallet support is expected to be phased out, but if isactive is only different from ismine in legacy wallets, is it worth adding a field (and all the new methods), versus just updating the ismine field documentation in the help for descriptor wallets? (When I'm double-checking an address with getaddressinfo before using it to receive, ismine is a field I'm already checking.)
@jonatack after diving back in to this issue I realized that `ismine != isactive` for
...
⚠️ ishaanam opened an issue: "Creating a Wallet Feature Guidelines Document"
(https://github.com/bitcoin/bitcoin/issues/28062)
It would be useful to the project if there was a document detailing what features are considered "out of scope" of the Bitcoin Core Wallet. This would make evaluating wallet PRs that add features easier for reviewers. It would also be useful to contributors so that they can get a better sense of which features are more likely to get merged. I'm curious if anybody else thinks that such a document would be useful, and if there are any guidelines that should be added. These guidelines would probabl
...
(https://github.com/bitcoin/bitcoin/issues/28062)
It would be useful to the project if there was a document detailing what features are considered "out of scope" of the Bitcoin Core Wallet. This would make evaluating wallet PRs that add features easier for reviewers. It would also be useful to contributors so that they can get a better sense of which features are more likely to get merged. I'm curious if anybody else thinks that such a document would be useful, and if there are any guidelines that should be added. These guidelines would probabl
...
💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629250320)
> Don't revert the commit; just remove the stray file and squash.
I think it should be fine now, can you please check once again?
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629250320)
> Don't revert the commit; just remove the stray file and squash.
I think it should be fine now, can you please check once again?
💬 sipa commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629252705)
You need to squash the commits. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629252705)
You need to squash the commits. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 ItIsOHM commented on pull request "[WIP] rpc: doc: Added `longpollid` and `data` params to `template_request` #27998":
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629255377)
> You need to squash the commits. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
I think I did squash them, atleast that's what it shows in the commits tab.
(https://github.com/bitcoin/bitcoin/pull/28056#issuecomment-1629255377)
> You need to squash the commits. See https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
I think I did squash them, atleast that's what it shows in the commits tab.