👍 maflcko approved a pull request: "Remove version field from GetSerializeSize"
(https://github.com/bitcoin/bitcoin/pull/28878#pullrequestreview-1734526484)
Some nits in the first commit. Otherwise:
ACK 83986f464c59a6517f790a960a72574e167f3f72 📒
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
tr
...
(https://github.com/bitcoin/bitcoin/pull/28878#pullrequestreview-1734526484)
Some nits in the first commit. Otherwise:
ACK 83986f464c59a6517f790a960a72574e167f3f72 📒
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
tr
...
💬 maflcko commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395782019)
nit in efa9eb6d7c8012fe4ed85699d81c8fe5dd18da1e: `inline` is implied by `template` and can be removed.
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395782019)
nit in efa9eb6d7c8012fe4ed85699d81c8fe5dd18da1e: `inline` is implied by `template` and can be removed.
💬 maflcko commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395784487)
Why remove the `explicit`? Shouldn't matter in this context, but when in doubt, I'd prefer to keep it, unless there is a reason to remove it?
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395784487)
Why remove the `explicit`? Shouldn't matter in this context, but when in doubt, I'd prefer to keep it, unless there is a reason to remove it?
💬 maflcko commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395789229)
Same (`inline`)
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395789229)
Same (`inline`)
💬 maflcko commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395786545)
unrelated nit in the first commit: Can remove the `()`, which achieve nothing in this context.
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1395786545)
unrelated nit in the first commit: Can remove the `()`, which achieve nothing in this context.
💬 maflcko commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1814649851)
Ah, looks like it was never merged https://github.com/bitcoin/bitcoin/pull/23444
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1814649851)
Ah, looks like it was never merged https://github.com/bitcoin/bitcoin/pull/23444
💬 laanwj commented on pull request "Fix SSE4.1-related issues":
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-1814651122)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-1814651122)
Concept ACK
💬 dergoegge commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1814654792)
> Ah, looks like it was never merged https://github.com/bitcoin/bitcoin/pull/23444
Soooo, should we delete it and then someone can re-introduce with #23444 applied + perf improvements?
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1814654792)
> Ah, looks like it was never merged https://github.com/bitcoin/bitcoin/pull/23444
Soooo, should we delete it and then someone can re-introduce with #23444 applied + perf improvements?
💬 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#discussion_r1395881820)
This is unambiguous, with the following decoding rules:
* Decimal numbers are turned into `OP_n` if applicable, otherwise into a direct push of the minimally-encoded form of that number.
* `<...>` is turned into a direct push if up to 75 bytes, into `OP_PUSHDATA1` if below 256 bytes, into `OP_PUSHDATA2` if below 65536 bytes, and into `OP_PUSHDATA4` otherwise.
* `OPCODE<...>` is turned into a push using the relevant opcode.
(https://github.com/bitcoin/bitcoin/pull/28824#discussion_r1395881820)
This is unambiguous, with the following decoding rules:
* Decimal numbers are turned into `OP_n` if applicable, otherwise into a direct push of the minimally-encoded form of that number.
* `<...>` is turned into a direct push if up to 75 bytes, into `OP_PUSHDATA1` if below 256 bytes, into `OP_PUSHDATA2` if below 65536 bytes, and into `OP_PUSHDATA4` otherwise.
* `OPCODE<...>` is turned into a push using the relevant opcode.
💬 sipa commented on issue "v26.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1814684166)
Could #28440 be related?
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1814684166)
Could #28440 be related?
💬 jonatack commented on pull request "[26.x] Final Changes":
(https://github.com/bitcoin/bitcoin/pull/28872#issuecomment-1814691849)
I see the release notes have been added since my feedback above.
If the regression in v26.0 is left unfixed, it would be a good idea to add a warning to the "Notable Changes" section of the release notes where the changed behavior is described, as the way nodes have until now been able to ensure connections to various networks and particularly the ones with fewer peers, via addnode, is the behavior that is impacted. The regression was first reported in August in https://github.com/bitcoin/bi
...
(https://github.com/bitcoin/bitcoin/pull/28872#issuecomment-1814691849)
I see the release notes have been added since my feedback above.
If the regression in v26.0 is left unfixed, it would be a good idea to add a warning to the "Notable Changes" section of the release notes where the changed behavior is described, as the way nodes have until now been able to ensure connections to various networks and particularly the ones with fewer peers, via addnode, is the behavior that is impacted. The regression was first reported in August in https://github.com/bitcoin/bi
...
💬 dergoegge commented on pull request "Fix SSE4.1-related issues":
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-1814695279)
Cool! Concept ACK
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-1814695279)
Cool! Concept ACK
👋 hebasto's pull request is ready for review: "Fix SSE4.1-related issues"
(https://github.com/bitcoin/bitcoin/pull/28893)
(https://github.com/bitcoin/bitcoin/pull/28893)
💬 brandonblack commented on issue "MuSig2 support":
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1814703426)
> For MuSig2, each signer needs all pubkeys and its secret key in order to sign.
To clarify, this is within _spec_ MuSig2 as described in BIP327. @AdamISZ's construction is quite possibly also secure and correct, but it's _way_ above my pay grade to evaluate the security or correctness of methods outside the spec :-P
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1814703426)
> For MuSig2, each signer needs all pubkeys and its secret key in order to sign.
To clarify, this is within _spec_ MuSig2 as described in BIP327. @AdamISZ's construction is quite possibly also secure and correct, but it's _way_ above my pay grade to evaluate the security or correctness of methods outside the spec :-P
💬 maflcko commented on issue "v26.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1814729772)
> All binaries are downloaded from https://bitcoincore.org/bin/.
Does it also happen if you compile from source? If yes, you could try bisecting.
(https://github.com/bitcoin/bitcoin/issues/28718#issuecomment-1814729772)
> All binaries are downloaded from https://bitcoincore.org/bin/.
Does it also happen if you compile from source? If yes, you could try bisecting.
💬 instagibbs commented on issue "wallet: Imports with pre-existing balance somtimes don't have any balance after a rescan":
(https://github.com/bitcoin/bitcoin/issues/19808#issuecomment-1814746667)
I think I hit a version of this on 26.0 rc2.
It seems to have found transactions from 2023 in blocks, but nothing prior, leaving everything else "unconfirmed".
(https://github.com/bitcoin/bitcoin/issues/19808#issuecomment-1814746667)
I think I hit a version of this on 26.0 rc2.
It seems to have found transactions from 2023 in blocks, but nothing prior, leaving everything else "unconfirmed".
⚠️ maflcko reopened an issue: "wallet: Imports with pre-existing balance somtimes don't have any balance after a rescan"
(https://github.com/bitcoin/bitcoin/issues/19808)
This bug has been around for a while (at least 0.17 from what I remember) but I guess no one opened an issue for it. I also have been unable to debug this at all.
Sometimes, when importing public keys and scripts which already have a balance, the balance is not detected after the import's rescan finishes. This has particularly occurred with the use of `importmulti` (but I suspect it should happen with the other `import` commands, I just don't use those). Even with the timestamp set correctly
...
(https://github.com/bitcoin/bitcoin/issues/19808)
This bug has been around for a while (at least 0.17 from what I remember) but I guess no one opened an issue for it. I also have been unable to debug this at all.
Sometimes, when importing public keys and scripts which already have a balance, the balance is not detected after the import's rescan finishes. This has particularly occurred with the use of `importmulti` (but I suspect it should happen with the other `import` commands, I just don't use those). Even with the timestamp set correctly
...
💬 kevkevinpal commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#issuecomment-1814765520)
## Below are commits and corresponding comments they address
#### [80f7ccd](https://github.com/kevkevinpal/bitcoin/pull/6/commits/80f7ccdefda97cba9a7a913bf9c8ae19e368f7d9) test: Directly constructing 2 entry map for getprioritisedtransactions
- https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219668280
- https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219669146
- https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219669511
#### [be75730](https://github.com/k
...
(https://github.com/bitcoin/bitcoin/pull/28885#issuecomment-1814765520)
## Below are commits and corresponding comments they address
#### [80f7ccd](https://github.com/kevkevinpal/bitcoin/pull/6/commits/80f7ccdefda97cba9a7a913bf9c8ae19e368f7d9) test: Directly constructing 2 entry map for getprioritisedtransactions
- https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219668280
- https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219669146
- https://github.com/bitcoin/bitcoin/pull/27501#discussion_r1219669511
#### [be75730](https://github.com/k
...
💬 AdamISZ commented on issue "MuSig2 support":
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1814769006)
> To clarify, this is within _spec_ MuSig2 as described in BIP327. @AdamISZ's construction is quite possibly also secure and correct, but it's _way_ above my pay grade to evaluate the security or correctness of methods outside the spec :-P
This would be a good time to mention that - even though it's sound advice to never step outside the procedure of a spec like this, because this is cryptography - there's a different reason why my suggestion there is "flawed", aside from the a) b) c) that I
...
(https://github.com/bitcoin/bitcoin/issues/23326#issuecomment-1814769006)
> To clarify, this is within _spec_ MuSig2 as described in BIP327. @AdamISZ's construction is quite possibly also secure and correct, but it's _way_ above my pay grade to evaluate the security or correctness of methods outside the spec :-P
This would be a good time to mention that - even though it's sound advice to never step outside the procedure of a spec like this, because this is cryptography - there's a different reason why my suggestion there is "flawed", aside from the a) b) c) that I
...
👍 dergoegge approved a pull request: "fuzz: AutoFile with XOR"
(https://github.com/bitcoin/bitcoin/pull/28873#pullrequestreview-1734852285)
ACK faa25718b3f11f24aa41f0968bbd4da104814bc5
Individual coverage after brief fuzzing:
- https://dergoegge.github.io/bitcoin-coverage/pr28873-buffered_file/fuzz.coverage/src/streams.cpp.gcov.html
- https://dergoegge.github.io/bitcoin-coverage/pr28873-autofile/fuzz.coverage/src/streams.cpp.gcov.html
(https://github.com/bitcoin/bitcoin/pull/28873#pullrequestreview-1734852285)
ACK faa25718b3f11f24aa41f0968bbd4da104814bc5
Individual coverage after brief fuzzing:
- https://dergoegge.github.io/bitcoin-coverage/pr28873-buffered_file/fuzz.coverage/src/streams.cpp.gcov.html
- https://dergoegge.github.io/bitcoin-coverage/pr28873-autofile/fuzz.coverage/src/streams.cpp.gcov.html