💬 brunoerg commented on pull request "fuzz: rule-out too deep derivation paths in descriptor parsing targets":
(https://github.com/bitcoin/bitcoin/pull/28832#discussion_r1394252904)
Correct me if I'm wrong, but wouldn't `HasDeepDerivPath` avoid descriptors like `pkh([d34db33f/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*)`? Is this intended?
(https://github.com/bitcoin/bitcoin/pull/28832#discussion_r1394252904)
Correct me if I'm wrong, but wouldn't `HasDeepDerivPath` avoid descriptors like `pkh([d34db33f/44'/0'/0']xpub6ERApfZwUNrhLCkDtcHTcxd75RbzS1ed54G1LkBUHQVHQKqhMkhgbmJbZRkrgZw4koxb5JaHWkY4ALHY2grBGRjaDMzQLcgJvLJuZZvRcEL/1/*)`? Is this intended?
📝 dergoegge opened a pull request: "fuzz: Delete wallet_notifications"
(https://github.com/bitcoin/bitcoin/pull/28882)
This harness is not doing much since it is incredibly slow. It's probably better to remove it until someone finds the time to improve it. Otherwise this just wastes resources in our CI and oss-fuzz.
On oss-fuzz for example, it never even starts fuzzing because it times-out while running through the existing corpus:
```
Component revisions (build r202311100604):
Bitcoin-core: b3898e946cf81e2e7b573e1c5204bd29af2feecd
Botan: 201cfe586e6d529360fbcde6f216f1da0c3db48f
Cryptofuzz: 537263836ea
...
(https://github.com/bitcoin/bitcoin/pull/28882)
This harness is not doing much since it is incredibly slow. It's probably better to remove it until someone finds the time to improve it. Otherwise this just wastes resources in our CI and oss-fuzz.
On oss-fuzz for example, it never even starts fuzzing because it times-out while running through the existing corpus:
```
Component revisions (build r202311100604):
Bitcoin-core: b3898e946cf81e2e7b573e1c5204bd29af2feecd
Botan: 201cfe586e6d529360fbcde6f216f1da0c3db48f
Cryptofuzz: 537263836ea
...
💬 willcl-ark commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1812610712)
Currently `GetOpCode()` will return `OP_UNKNOWN` for unknown opcodes which seems reasonable. Dropping the `OP_` would appear in the asm as `UNKNOWN`.
As for undecodable bytes, if we did want a keyword, perhaps something more explicit like `UNDECODABLE<...>` would be better? We are essentially using what I would expect `RAW` to denote for > 5 byte minimal pushes if we go with this:
> Any other minimal pushes (so direct push <= 75 bytes, PUSHDATA1 > 75 bytes, PUSHDATA2 > 255 bytes) become <.
...
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1812610712)
Currently `GetOpCode()` will return `OP_UNKNOWN` for unknown opcodes which seems reasonable. Dropping the `OP_` would appear in the asm as `UNKNOWN`.
As for undecodable bytes, if we did want a keyword, perhaps something more explicit like `UNDECODABLE<...>` would be better? We are essentially using what I would expect `RAW` to denote for > 5 byte minimal pushes if we go with this:
> Any other minimal pushes (so direct push <= 75 bytes, PUSHDATA1 > 75 bytes, PUSHDATA2 > 255 bytes) become <.
...
💬 sipa commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1812617163)
> Currently GetOpCode() will return OP_UNKNOWN for unknown opcodes which seems reasonable.
That's trivially ambiguous, as *all* unknown opcodes are mapped to the same thing.
> As for undecodable bytes, if we did want a keyword, perhaps something more explicit like UNDECODABLE<...> would be better?
Maybe, but I wouldn't use `<...>` because it's not a push.
> We are essentially using what I would expect RAW to denote for > 5 byte minimal pushes if we go with this:
No, *any* pushes b
...
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1812617163)
> Currently GetOpCode() will return OP_UNKNOWN for unknown opcodes which seems reasonable.
That's trivially ambiguous, as *all* unknown opcodes are mapped to the same thing.
> As for undecodable bytes, if we did want a keyword, perhaps something more explicit like UNDECODABLE<...> would be better?
Maybe, but I wouldn't use `<...>` because it's not a push.
> We are essentially using what I would expect RAW to denote for > 5 byte minimal pushes if we go with this:
No, *any* pushes b
...
💬 maflcko commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1812622711)
> This harness is not doing much since it is incredibly slow. It's probably better to remove it until someone finds the time to improve it. Otherwise this just wastes resources in our CI and oss-fuzz.
I think it was added as a regression fuzz test, so it can find at least that one regression. I am running it on my servers and I agree it should be improved.
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1812622711)
> This harness is not doing much since it is incredibly slow. It's probably better to remove it until someone finds the time to improve it. Otherwise this just wastes resources in our CI and oss-fuzz.
I think it was added as a regression fuzz test, so it can find at least that one regression. I am running it on my servers and I agree it should be improved.
💬 mzumsande commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812702510)
> Note also, that item 7 in the PR description involves a regression in v26.x.
I don't think that this can be called a regression:
* There is nothing new about the root problem (making automatic connections to addnode peers in rare cases) in v26.
* It can happen that such a peer could now be protected as the only one of its network, but to determine if it's a regression this needs to be compared with the status before.
* Before we did protection-by-network, we would only ever get into the
...
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812702510)
> Note also, that item 7 in the PR description involves a regression in v26.x.
I don't think that this can be called a regression:
* There is nothing new about the root problem (making automatic connections to addnode peers in rare cases) in v26.
* It can happen that such a peer could now be protected as the only one of its network, but to determine if it's a regression this needs to be compared with the status before.
* Before we did protection-by-network, we would only ever get into the
...
💬 brunoerg commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1812720492)
Concept ACK
I started working on improving it, perhaps I could prioritize it. Besides being slow, I believe it is not deterministic.
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1812720492)
Concept ACK
I started working on improving it, perhaps I could prioritize it. Besides being slow, I believe it is not deterministic.
💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812724875)
@mzumsande The improved logging allows distinguishing when this is due to the new protection logic, and with occasional exceptions, it is.
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812724875)
@mzumsande The improved logging allows distinguishing when this is due to the new protection logic, and with occasional exceptions, it is.
🚀 fanquake merged a pull request: "Use serialization parameters for CTransaction"
(https://github.com/bitcoin/bitcoin/pull/28438)
(https://github.com/bitcoin/bitcoin/pull/28438)
🚀 fanquake merged a pull request: "guix: update signapple (drop macho & altgraph)"
(https://github.com/bitcoin/bitcoin/pull/28859)
(https://github.com/bitcoin/bitcoin/pull/28859)
💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812748534)
> I'd guess you could probably have waited for months to actually evict a mis-placed peer
In my testing it is frequent and systematic with our networks having a smaller number of peers. I believe these are the networks we're most interested in protecting with the new logic, and the ones for which users are likely to have addnode entries to ensure connection to the network.
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812748534)
> I'd guess you could probably have waited for months to actually evict a mis-placed peer
In my testing it is frequent and systematic with our networks having a smaller number of peers. I believe these are the networks we're most interested in protecting with the new logic, and the ones for which users are likely to have addnode entries to ensure connection to the network.
💬 mzumsande commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812752979)
> @mzumsande The improved logging allows distinguishing when this is due to the new protection logic, and with occasional exceptions, it is.
That's not my point. My point (see above) is that in earlier versions, the extra-outbound eviction (which would only happen in the stale-tip case then) was not a working or reliable mechanism to evict these peers, so there is no regression. Or did I miss something in my above explanation / do you have logs that pre-26 the stale-tip outbound eviction coul
...
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812752979)
> @mzumsande The improved logging allows distinguishing when this is due to the new protection logic, and with occasional exceptions, it is.
That's not my point. My point (see above) is that in earlier versions, the extra-outbound eviction (which would only happen in the stale-tip case then) was not a working or reliable mechanism to evict these peers, so there is no regression. Or did I miss something in my above explanation / do you have logs that pre-26 the stale-tip outbound eviction coul
...
💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1812753022)
@ajtowns To avoid duplicate work, it would be good if you opened all follow-up pulls you wanted to do, or reply with a list here. Others can then take the remaining follow-ups :)
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1812753022)
@ajtowns To avoid duplicate work, it would be good if you opened all follow-up pulls you wanted to do, or reply with a list here. Others can then take the remaining follow-ups :)
💬 maflcko commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1812754317)
> I believe it is not deterministic.
Can you add steps to reproduce?
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1812754317)
> I believe it is not deterministic.
Can you add steps to reproduce?
💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812773694)
Empirically, the regression is that we're actively adding addnode peers as outbound ones, and keeping them there. Since a couple of years I run a node with all three of our privacy networks and addnode entries for each, along with clearnet, and I didn't see my addnode peers being connected to as non-manual outbounds. This is new behavior for my node.
Aside, I'm adding unit tests for each change right now, and these are finding additional issues.
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812773694)
Empirically, the regression is that we're actively adding addnode peers as outbound ones, and keeping them there. Since a couple of years I run a node with all three of our privacy networks and addnode entries for each, along with clearnet, and I didn't see my addnode peers being connected to as non-manual outbounds. This is new behavior for my node.
Aside, I'm adding unit tests for each change right now, and these are finding additional issues.
🤔 furszy reviewed a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1732331088)
Nice findings 👌🏼.
I haven't delved into investigating the reported BDB freeing issue within this context yet. But, can report that in the branch where I changed the underlying db migration mechanism ([link](https://github.com/furszy/bitcoin-core/tree/2023_wallet_safer_migration)), I haven't encountered the issue. I just pulled and ran all this PR new tests there and they all pass successfully in a loop.
One of the ideas of this work is to fix failures like 65b2da9 and others in a safer m
...
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1732331088)
Nice findings 👌🏼.
I haven't delved into investigating the reported BDB freeing issue within this context yet. But, can report that in the branch where I changed the underlying db migration mechanism ([link](https://github.com/furszy/bitcoin-core/tree/2023_wallet_safer_migration)), I haven't encountered the issue. I just pulled and ran all this PR new tests there and they all pass successfully in a loop.
One of the ideas of this work is to fix failures like 65b2da9 and others in a safer m
...
👍 hebasto approved a pull request: "doc: remove mention of missing bdb being a configure error"
(https://github.com/bitcoin/bitcoin/pull/28881#pullrequestreview-1732347366)
ACK 30bd4b1e4aee00edbe77da7c20bf80e28f0561cc.
(https://github.com/bitcoin/bitcoin/pull/28881#pullrequestreview-1732347366)
ACK 30bd4b1e4aee00edbe77da7c20bf80e28f0561cc.
📝 muxator opened a pull request: "contrib: use a raw string for a regular expression literal that contains backslashes in signet/miner"
(https://github.com/bitcoin/bitcoin/pull/28883)
Running `contrib/signet/miner` under python >= 3.12 causes a `SyntaxWarning`. The problem was already present in previous versions, but it only triggered a `DeprecationWarning`, which was not shown by default.
The change is useful for future-proofing the code base, since future python versions will start to exit with a runtime exception (see the reference given later).
Command to see the warning at runtime under python3.11 (`DeprecationWarning`, needs "-Walways"):
```
$ python3.11 -Walwa
...
(https://github.com/bitcoin/bitcoin/pull/28883)
Running `contrib/signet/miner` under python >= 3.12 causes a `SyntaxWarning`. The problem was already present in previous versions, but it only triggered a `DeprecationWarning`, which was not shown by default.
The change is useful for future-proofing the code base, since future python versions will start to exit with a runtime exception (see the reference given later).
Command to see the warning at runtime under python3.11 (`DeprecationWarning`, needs "-Walways"):
```
$ python3.11 -Walwa
...
💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1394406381)
Yes, I'm going to punt on this commit for now. Seeing dozens to hundreds of connection attempts per day from, say, 1-2 inbound I2P peers that are already connected. However, I'm not sure these peers are malicious, and the behavior might be due to other issues in our already connected detection or outbound logic addressed in this PR.
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1394406381)
Yes, I'm going to punt on this commit for now. Seeing dozens to hundreds of connection attempts per day from, say, 1-2 inbound I2P peers that are already connected. However, I'm not sure these peers are malicious, and the behavior might be due to other issues in our already connected detection or outbound logic addressed in this PR.
💬 maflcko commented on pull request "contrib: use a raw string for a regular expression literal that contains backslashes in signet/miner":
(https://github.com/bitcoin/bitcoin/pull/28883#issuecomment-1812805851)
lgtm ACK defdf67765a3d757f4d3840602eef7ccdac9bb49
(https://github.com/bitcoin/bitcoin/pull/28883#issuecomment-1812805851)
lgtm ACK defdf67765a3d757f4d3840602eef7ccdac9bb49