💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942646524)
`legacy`
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942646524)
`legacy`
💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942794684)
`// Invalid scripts such as P2SH-P2SH and P2WSH-P2SH, among others, will be added as candidates.`
In the below function, I notice script itself, P2SH, P2WSH, P2SH-P2WSH being added only. These invalid scripts could be added as candidates because the underlying `script` could be a P2SH?
With this script nesting involved, it makes me want to read the logic of `IsMine` to see how it handles it!
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942794684)
`// Invalid scripts such as P2SH-P2SH and P2WSH-P2SH, among others, will be added as candidates.`
In the below function, I notice script itself, P2SH, P2WSH, P2SH-P2WSH being added only. These invalid scripts could be added as candidates because the underlying `script` could be a P2SH?
With this script nesting involved, it makes me want to read the logic of `IsMine` to see how it handles it!
💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942786866)
`contain`
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942786866)
`contain`
💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942841714)
Nit:
```diff
- if (!Assume(spks.size() == 0)) {
+ if (!Assume(spks.empty())) {
```
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942841714)
Nit:
```diff
- if (!Assume(spks.size() == 0)) {
+ if (!Assume(spks.empty())) {
```
💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942897067)
`have`
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942897067)
`have`
💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942865073)
Thanks for this comment, very helpful!
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942865073)
Thanks for this comment, very helpful!
💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942990827)
`self.old_node`?
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1942990827)
`self.old_node`?
💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1943021410)
Where was the first check?
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1943021410)
Where was the first check?
💬 rkrux commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1943008904)
Nit: Can add a corresponding call before sending the transaction that checks the balance is 6.
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1943008904)
Nit: Can add a corresponding call before sending the transaction that checks the balance is 6.
🤔 sipa reviewed a pull request: "tracing: network connection tracepoints"
(https://github.com/bitcoin/bitcoin/pull/25832#pullrequestreview-2596026595)
utACK e3622a969293feea75cfadc8f7c6083edcd6d8de
(https://github.com/bitcoin/bitcoin/pull/25832#pullrequestreview-2596026595)
utACK e3622a969293feea75cfadc8f7c6083edcd6d8de
💬 sipa commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1943088145)
It's not very nice to expose this internal value in the interface, if we want it to be stable. What about using the [BIP155 network ID](https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki#user-content-Specification) instead?
(https://github.com/bitcoin/bitcoin/pull/25832#discussion_r1943088145)
It's not very nice to expose this internal value in the interface, if we want it to be stable. What about using the [BIP155 network ID](https://github.com/bitcoin/bips/blob/master/bip-0155.mediawiki#user-content-Specification) instead?
📝 hebasto opened a pull request: "depends: Avoid using the `-ffile-prefix-map` compiler option"
(https://github.com/bitcoin/bitcoin/pull/31800)
This PR is similar to https://github.com/bitcoin/bitcoin/pull/31337 and applies analogous changes to all dependency packages.
The issue was [recently noticed](https://github.com/bitcoin/bitcoin/pull/31661#discussion_r1923896475) when `-ffile-prefix-map` was added to the `libevent` package, which is built in OSS-Fuzz.
This PR replaces `-ffile-prefix-map` from in packages for consistency.
(https://github.com/bitcoin/bitcoin/pull/31800)
This PR is similar to https://github.com/bitcoin/bitcoin/pull/31337 and applies analogous changes to all dependency packages.
The issue was [recently noticed](https://github.com/bitcoin/bitcoin/pull/31661#discussion_r1923896475) when `-ffile-prefix-map` was added to the `libevent` package, which is built in OSS-Fuzz.
This PR replaces `-ffile-prefix-map` from in packages for consistency.
💬 hebasto commented on issue "fuzz: oss-fuzz coverage build is failing":
(https://github.com/bitcoin/bitcoin/issues/31770#issuecomment-2637098504)
A fix has been proposed in https://github.com/bitcoin/bitcoin/pull/31800.
(https://github.com/bitcoin/bitcoin/issues/31770#issuecomment-2637098504)
A fix has been proposed in https://github.com/bitcoin/bitcoin/pull/31800.
💬 instagibbs commented on pull request "test: fixes p2p_ibd_txrelay wait time":
(https://github.com/bitcoin/bitcoin/pull/31759#issuecomment-2637099528)
meta: do we ever want a test to bumpmocktime without setting it first?
(https://github.com/bitcoin/bitcoin/pull/31759#issuecomment-2637099528)
meta: do we ever want a test to bumpmocktime without setting it first?
💬 luke-jr commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637110634)
Yes, I think it would be better.
I would just have the git clone command documented though, no need to automate it?
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2637110634)
Yes, I think it would be better.
I would just have the git clone command documented though, no need to automate it?
💬 polespinasa commented on pull request "rpc, logging: return "verificationprogress" of 1 when up to date":
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1943113886)
> Maybe that's how it is used at the moment, but the documentation of the function implies that the estimate can be used on any block, not that pindex necessarily is the latest block.
As long as we have good headers using any block should not be a problem and should return a reasonable estimate.
> That seems bad to me. Being isolated from the network shouldn't make it report values _that_ far off! The old current-time-based estimate might have edge cases around 1, but apart from that it
...
(https://github.com/bitcoin/bitcoin/pull/31177#discussion_r1943113886)
> Maybe that's how it is used at the moment, but the documentation of the function implies that the estimate can be used on any block, not that pindex necessarily is the latest block.
As long as we have good headers using any block should not be a problem and should return a reasonable estimate.
> That seems bad to me. Being isolated from the network shouldn't make it report values _that_ far off! The old current-time-based estimate might have edge cases around 1, but apart from that it
...
🤔 hodlinator reviewed a pull request: "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`"
(https://github.com/bitcoin/bitcoin/pull/31734#pullrequestreview-2588406601)
Code review 498c47d9315ef79c107460b678f834d19cb9a53e
#### Since last review
`git range-diff master 0bc213d 498c47d`
- Reordered commits and avoid touching new test code in 2 commits. ✅
- Forwards on `cache` argument from calling function, changed types and `const`. ✅
- Improved PR title and summary. ✅
#### Concept
Before the PR, first key origin/derivation path in the descriptor undergoes `'` -> `h` conversion, but the second one does not. There seems to be some code in *descrip
...
(https://github.com/bitcoin/bitcoin/pull/31734#pullrequestreview-2588406601)
Code review 498c47d9315ef79c107460b678f834d19cb9a53e
#### Since last review
`git range-diff master 0bc213d 498c47d`
- Reordered commits and avoid touching new test code in 2 commits. ✅
- Forwards on `cache` argument from calling function, changed types and `const`. ✅
- Improved PR title and summary. ✅
#### Concept
Before the PR, first key origin/derivation path in the descriptor undergoes `'` -> `h` conversion, but the second one does not. There seems to be some code in *descrip
...
💬 hodlinator commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1938347218)
```suggestion
const DescriptorCache* cache LIFETIMEBOUND)
```
Not sure how much it actually helps, especially on raw pointers, but at least `LIFETIMEBOUND` documents expectations, and is already used on the first arg which is also a raw pointer. See: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lifetimebound
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1938347218)
```suggestion
const DescriptorCache* cache LIFETIMEBOUND)
```
Not sure how much it actually helps, especially on raw pointers, but at least `LIFETIMEBOUND` documents expectations, and is already used on the first arg which is also a raw pointer. See: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#lifetimebound
💬 hodlinator commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1943031412)
nit: When debugging the test, it was annoying that both of the keys had identical beginnings (`[1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/`), deriving them from different master keys with different derivation paths would help keep them apart.
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1943031412)
nit: When debugging the test, it was annoying that both of the keys had identical beginnings (`[1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/`), deriving them from different master keys with different derivation paths would help keep them apart.
💬 hodlinator commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1938349220)
nit: More readable if you think it's tolerable to use double-quotes in this one case.
```suggestion
'desc': "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))#xhrh0cvn",
```
(https://github.com/bitcoin/bitcoin/pull/31734#discussion_r1938349220)
nit: More readable if you think it's tolerable to use double-quotes in this one case.
```suggestion
'desc': "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))#xhrh0cvn",
```