💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1679633885)
Added a help text.
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1679633885)
Added a help text.
🤔 ismaelsadeeq reviewed a pull request: "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending"
(https://github.com/bitcoin/bitcoin/pull/30352#pullrequestreview-2180579805)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/30352#pullrequestreview-2180579805)
Concept ACK
💬 ismaelsadeeq commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679609544)
whats the `0x02`?
P2Anchor is `OP_TRUE <0x4e73>`
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679609544)
whats the `0x02`?
P2Anchor is `OP_TRUE <0x4e73>`
💬 ismaelsadeeq commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679615452)
```suggestion
ANCHOR, //!< anyone can spend script
```
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679615452)
```suggestion
ANCHOR, //!< anyone can spend script
```
💬 ismaelsadeeq commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679638523)
A test for this will be nice!
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679638523)
A test for this will be nice!
💬 ismaelsadeeq commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679627045)
In 7013da222720fde5ea1c76bba99b1c91ae18558f "policy: Add OP_TRUE <0x4e73> as a standard output type"
Add `ANCHOR` txout script type description in the `CTxDestination` docstring.
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679627045)
In 7013da222720fde5ea1c76bba99b1c91ae18558f "policy: Add OP_TRUE <0x4e73> as a standard output type"
Add `ANCHOR` txout script type description in the `CTxDestination` docstring.
💬 theStack commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1679647788)
AFAICT this assert would lead to a crash if a tx is scanned with a taproot output where the x-only-pubkey is not on the curve (note that such outputs adhere to standardness rules, i.e. it's trivial to create such a tx, and probably there are already a good amount of them on mainnet). Proposed fix, not tested yet:
```suggestion
if (!secp256k1_xonly_pubkey_parse(secp256k1_context_static, &tx_output_obj, txoutputs[i].data())) {
continue;
}
```
For consistency reaso
...
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1679647788)
AFAICT this assert would lead to a crash if a tx is scanned with a taproot output where the x-only-pubkey is not on the curve (note that such outputs adhere to standardness rules, i.e. it's trivial to create such a tx, and probably there are already a good amount of them on mainnet). Proposed fix, not tested yet:
```suggestion
if (!secp256k1_xonly_pubkey_parse(secp256k1_context_static, &tx_output_obj, txoutputs[i].data())) {
continue;
}
```
For consistency reaso
...
👍 maflcko approved a pull request: "fix: Make TxidFromString() respect string_view length"
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2180659057)
ACK c3a9c71c7077324bf0aa40f834f7537a14619340 🐞
<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=
trusted comment: ACK c3a9c71c7077324bf0aa40f834
...
(https://github.com/bitcoin/bitcoin/pull/30436#pullrequestreview-2180659057)
ACK c3a9c71c7077324bf0aa40f834f7537a14619340 🐞
<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=
trusted comment: ACK c3a9c71c7077324bf0aa40f834
...
💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679665692)
It's the instruction to push next two bytes on the stack
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679665692)
It's the instruction to push next two bytes on the stack
💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679666497)
should have coverage, please check it fails :)
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1679666497)
should have coverage, please check it fails :)
💬 sipa commented on pull request "Fix SSE4.1-related issues":
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-2231284275)
utACK d440f13db02c82c842000abe4fe4d0c721a4ad3b
(https://github.com/bitcoin/bitcoin/pull/28893#issuecomment-2231284275)
utACK d440f13db02c82c842000abe4fe4d0c721a4ad3b
💬 theuni commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1679674217)
In commit ff368465a66298487a88d8da575f5251d094d687
Could you remind me what's going on here, please? I assume these aren't needed because we're meant to be adding compile definitions to specific libs. But from what I can tell, we're not actually adding them to `sha256.cpp`? So... is this currently using an un-optimized sha2?
Generally I'd feel more comfortable turning these into opt-outs, I think, to avoid accidentally building without optims.
For the sake of review/sanity, I think for
...
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1679674217)
In commit ff368465a66298487a88d8da575f5251d094d687
Could you remind me what's going on here, please? I assume these aren't needed because we're meant to be adding compile definitions to specific libs. But from what I can tell, we're not actually adding them to `sha256.cpp`? So... is this currently using an un-optimized sha2?
Generally I'd feel more comfortable turning these into opt-outs, I think, to avoid accidentally building without optims.
For the sake of review/sanity, I think for
...
💬 fjahr commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#issuecomment-2231294972)
> maflcko's suggestion wouldn't remove the dependency. Unless you place the new enum somewhere else?. Which I don't think it worth it.
Yes, I was thinking of putting it in a file for types which we have been doing in the code base in several places. And @ryanofsky suggests introducing a `kernel/types.h` [here](https://github.com/bitcoin/bitcoin/pull/30214/commits/97cb62492903b188074de5c27b3dcba2c9b768ff) already. Moving code isn't hard to review and `chain.h` is big enough to be broken up a b
...
(https://github.com/bitcoin/bitcoin/pull/29770#issuecomment-2231294972)
> maflcko's suggestion wouldn't remove the dependency. Unless you place the new enum somewhere else?. Which I don't think it worth it.
Yes, I was thinking of putting it in a file for types which we have been doing in the code base in several places. And @ryanofsky suggests introducing a `kernel/types.h` [here](https://github.com/bitcoin/bitcoin/pull/30214/commits/97cb62492903b188074de5c27b3dcba2c9b768ff) already. Moving code isn't hard to review and `chain.h` is big enough to be broken up a b
...
👍 BrandonOdiwuor approved a pull request: "init: change shutdown order of load block thread and scheduler"
(https://github.com/bitcoin/bitcoin/pull/30435#pullrequestreview-2180709184)
Code Review ACK 5fd48360198d2ac49e43b24cc1469557b03567b8
(https://github.com/bitcoin/bitcoin/pull/30435#pullrequestreview-2180709184)
Code Review ACK 5fd48360198d2ac49e43b24cc1469557b03567b8
💬 hebasto commented on pull request "build: Introduce CMake-based buid system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1679690571)
> But from what I can tell, we're not actually adding them to `sha256.cpp`? So... is this currently using an un-optimized sha2?
Please note the `PUBLIC` keyword, which propagates definition as a usage requirement:
https://github.com/bitcoin/bitcoin/blob/2b7c0f4fdb81ec1cf0579fa71b37f56672934ab5/src/crypto/CMakeLists.txt#L125
and
https://github.com/bitcoin/bitcoin/blob/2b7c0f4fdb81ec1cf0579fa71b37f56672934ab5/src/crypto/CMakeLists.txt#L128
where that usage requirement is actually appl
...
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1679690571)
> But from what I can tell, we're not actually adding them to `sha256.cpp`? So... is this currently using an un-optimized sha2?
Please note the `PUBLIC` keyword, which propagates definition as a usage requirement:
https://github.com/bitcoin/bitcoin/blob/2b7c0f4fdb81ec1cf0579fa71b37f56672934ab5/src/crypto/CMakeLists.txt#L125
and
https://github.com/bitcoin/bitcoin/blob/2b7c0f4fdb81ec1cf0579fa71b37f56672934ab5/src/crypto/CMakeLists.txt#L128
where that usage requirement is actually appl
...
🤔 sipa reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2180269609)
ACK 21090d032f5c4d90a41c3ca2030690c75899ec1a
I have a few non-blocking suggested improvements in the new commit. I have also pushed a sanity check commit in https://github.com/sipa/bitcoin/commits/pr_28280, feel free to cherry-pick or squash in.
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2180269609)
ACK 21090d032f5c4d90a41c3ca2030690c75899ec1a
I have a few non-blocking suggested improvements in the new commit. I have also pushed a sanity check commit in https://github.com/sipa/bitcoin/commits/pr_28280, feel free to cherry-pick or squash in.
💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679414088)
In commit "coins: pass linked list of flagged entries to BatchWrite"
Can you add a one-paragraph doxygen description of what this type is for?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679414088)
In commit "coins: pass linked list of flagged entries to BatchWrite"
Can you add a one-paragraph doxygen description of what this type is for?
💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679416368)
In commit "coins: pass linked list of flagged entries to BatchWrite"
Can you add `LIFETIMEBOUND` to the `usage`, `sentinel`, and `map` arguments? That may make the compiler complain if the `CoinsViewCacheCursor` object is used after the arguments go out of scope.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679416368)
In commit "coins: pass linked list of flagged entries to BatchWrite"
Can you add `LIFETIMEBOUND` to the `usage`, `sentinel`, and `map` arguments? That may make the compiler complain if the `CoinsViewCacheCursor` object is used after the arguments go out of scope.
💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679484508)
In commit "coins: pass linked list of flagged entries to BatchWrite"
The behavior change is a bit confusing here, as `cursor.WillErase(*it)` will be true for spent items, which was not true for the old code's `erase`. Would it make sense to split this commit up in two; one part which introduces the cursor and only iterate over flagged entry, and then a second part which moves the deleting of spent entries from the separate loop in `CCoinsViewCache::Sync` call into the `CCoinsViewCache::BatchW
...
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679484508)
In commit "coins: pass linked list of flagged entries to BatchWrite"
The behavior change is a bit confusing here, as `cursor.WillErase(*it)` will be true for spent items, which was not true for the old code's `erase`. Would it make sense to split this commit up in two; one part which introduces the cursor and only iterate over flagged entry, and then a second part which moves the deleting of spent entries from the separate loop in `CCoinsViewCache::Sync` call into the `CCoinsViewCache::BatchW
...
💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679435737)
In commit "coins: pass linked list of flagged entries to BatchWrite"
I'd prefer a comment that explains what the flag does rather than just when to set it. For example "If will_erase is not set, iterating through the cursor will erase spent coins from the map, and other coins will be unflagged (removing them from the linked list). If will_erase is set, the underlying map will not be modified, as the caller is expected to wipe the entire map anyway."
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1679435737)
In commit "coins: pass linked list of flagged entries to BatchWrite"
I'd prefer a comment that explains what the flag does rather than just when to set it. For example "If will_erase is not set, iterating through the cursor will erase spent coins from the map, and other coins will be unflagged (removing them from the linked list). If will_erase is set, the underlying map will not be modified, as the caller is expected to wipe the entire map anyway."