✅ TheCharlatan closed a pull request: "kernel: Remove dependency on clientversion"
(https://github.com/bitcoin/bitcoin/pull/32543)
(https://github.com/bitcoin/bitcoin/pull/32543)
💬 TheCharlatan commented on pull request "kernel: Remove dependency on clientversion":
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-3528096559)
Closing this. Will pick it up again as part of a larger directional push to handle versioning in the kernel.
(https://github.com/bitcoin/bitcoin/pull/32543#issuecomment-3528096559)
Closing this. Will pick it up again as part of a larger directional push to handle versioning in the kernel.
💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2523702039)
Nice catch, we can simply check `hd_chain.nVersion` instead of having `auto hd_chain_version{legacy_data.GetHDChain().nVersion};`. We could also add an assert to ensure these two values are the same.
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2523702039)
Nice catch, we can simply check `hd_chain.nVersion` instead of having `auto hd_chain_version{legacy_data.GetHDChain().nVersion};`. We could also add an assert to ensure these two values are the same.
💬 brunoerg commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2523711918)
No null check because we always expect it to succeed given how the harness is built, but will add a check just to avoid UB.
(https://github.com/bitcoin/bitcoin/pull/32624#discussion_r2523711918)
No null check because we always expect it to succeed given how the harness is built, but will add a check just to avoid UB.
💬 mzumsande commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2523758447)
logging the block hash here is not as straightforward as it looks, because we would have to know which of the headers failed, and `ProcessNewBlockHeaders` doesn't return that info:
So I think we would either have to change the interface of `ProcessNewBlockHeaders`, or we would have to add a loop over `headers` (`CBlockHeader`) here, lookup each header in our block index and check whether it's `BLOCK_FAILED_MASK`.
I'm on vacation this week, will look into it more next week.
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2523758447)
logging the block hash here is not as straightforward as it looks, because we would have to know which of the headers failed, and `ProcessNewBlockHeaders` doesn't return that info:
So I think we would either have to change the interface of `ProcessNewBlockHeaders`, or we would have to add a loop over `headers` (`CBlockHeader`) here, lookup each header in our block index and check whether it's `BLOCK_FAILED_MASK`.
I'm on vacation this week, will look into it more next week.
📝 andrewtoth opened a pull request: "Remove incorrect lifetimebounds"
(https://github.com/bitcoin/bitcoin/pull/33870)
(https://github.com/bitcoin/bitcoin/pull/33870)
💬 maflcko commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2523765885)
Shouldn't be a blocker either way, because the block hash has been logged previously and it is just a matter of scrolling up
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2523765885)
Shouldn't be a blocker either way, because the block hash has been logged previously and it is just a matter of scrolling up
⚠️ juliamarvin74-alt opened an issue: "CERTIFIED BITCOIN AND ETH-USDT RECOVERY EXPERT WITH THE HELP OF TECHY FORCE CYBER RETRIEVAL"
(https://github.com/bitcoin/bitcoin/issues/33871)
I am writing to share a cautionary tale about the dangers of online impersonation and the importance of vigilance in the cryptocurrency space. Recently, I fell victim to a sophisticated scam perpetrated by individuals posing as representatives of Blockchain.com on Facebook. They convincingly replicated the company's official page, making it nearly impossible to distinguish between the genuine and fake pages. The scammers initiated contact with me through this fake page, gaining my trust by using
...
(https://github.com/bitcoin/bitcoin/issues/33871)
I am writing to share a cautionary tale about the dangers of online impersonation and the importance of vigilance in the cryptocurrency space. Recently, I fell victim to a sophisticated scam perpetrated by individuals posing as representatives of Blockchain.com on Facebook. They convincingly replicated the company's official page, making it nearly impossible to distinguish between the genuine and fake pages. The scammers initiated contact with me through this fake page, gaining my trust by using
...
💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2523795556)
Not exactly what I meant, I don't think that's ever called.
My objection was rather that the cache hierarchies have to stop somewhere and we're currently doing that by adding dummy values at the bottom via dummies, e.g.
```C++
CCoinsView viewDummy;
CCoinsViewCache view(&viewDummy);
```
But the base field is a pointer, which makes sense, the recursion has to stop somewhere.
https://github.com/bitcoin/bitcoin/blob/dfde31f2ec1f90976f3ba6b06f2b38a1307c01ab/src/coins.h#L344
But instead of a
...
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2523795556)
Not exactly what I meant, I don't think that's ever called.
My objection was rather that the cache hierarchies have to stop somewhere and we're currently doing that by adding dummy values at the bottom via dummies, e.g.
```C++
CCoinsView viewDummy;
CCoinsViewCache view(&viewDummy);
```
But the base field is a pointer, which makes sense, the recursion has to stop somewhere.
https://github.com/bitcoin/bitcoin/blob/dfde31f2ec1f90976f3ba6b06f2b38a1307c01ab/src/coins.h#L344
But instead of a
...
💬 theStack commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2523800662)
I see. Note that my comment was referring to your reply about why the `reject_reason` field is missing in the `ExtraWitness` invalid tx test case class, particularly about this sentence: _"Thus we don't receive any error message when submitting the transaction to the mempool."_. All txs specified in data/invalid_txs.py are submitted to mempool in `p2p_invalid_tx.py` and are expected to be rejected (unless `reject_reason` is set explicitly to `None`), otherwise this functional test would fail. I
...
(https://github.com/bitcoin/bitcoin/pull/31823#discussion_r2523800662)
I see. Note that my comment was referring to your reply about why the `reject_reason` field is missing in the `ExtraWitness` invalid tx test case class, particularly about this sentence: _"Thus we don't receive any error message when submitting the transaction to the mempool."_. All txs specified in data/invalid_txs.py are submitted to mempool in `p2p_invalid_tx.py` and are expected to be rejected (unless `reject_reason` is set explicitly to `None`), otherwise this functional test would fail. I
...
💬 theStack commented on pull request "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`":
(https://github.com/bitcoin/bitcoin/pull/31823#issuecomment-3528212892)
Concept ACK, happy to re-review after rebase
(https://github.com/bitcoin/bitcoin/pull/31823#issuecomment-3528212892)
Concept ACK, happy to re-review after rebase
💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2523797916)
How does this change affect the error catchers, e.g. https://github.com/bitcoin/bitcoin/blob/dfde31f2ec1f90976f3ba6b06f2b38a1307c01ab/src/coins.h#L507?
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2523797916)
How does this change affect the error catchers, e.g. https://github.com/bitcoin/bitcoin/blob/dfde31f2ec1f90976f3ba6b06f2b38a1307c01ab/src/coins.h#L507?
💬 alexanderwiederin commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2523835941)
I'd prefer 1. The benefits are:
a) Self-enforcing contract - the implementation maintains the 'same block' semantics regardless of internal changes.
b) Simpler reasoning - equality semantics are clear without needing much further context.
Regarding the pointer based approach: I don't think the performance improvement justifies coupling the API to Core's current internal architecture.
I can work with either approach - thought it was worth discussing the trade-offs.
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2523835941)
I'd prefer 1. The benefits are:
a) Self-enforcing contract - the implementation maintains the 'same block' semantics regardless of internal changes.
b) Simpler reasoning - equality semantics are clear without needing much further context.
Regarding the pointer based approach: I don't think the performance improvement justifies coupling the API to Core's current internal architecture.
I can work with either approach - thought it was worth discussing the trade-offs.
💬 maflcko commented on pull request "Remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3528262336)
missing refactor prefix in title?
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3528262336)
missing refactor prefix in title?
💬 maflcko commented on pull request "Remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3528289276)
review ACK 99d012ec80a4415e1a37218fb4933550276b9a0a 💧
<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: review ACK 99d012ec80a4
...
(https://github.com/bitcoin/bitcoin/pull/33870#issuecomment-3528289276)
review ACK 99d012ec80a4415e1a37218fb4933550276b9a0a 💧
<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: review ACK 99d012ec80a4
...
🤔 hebasto reviewed a pull request: "depends: Boost 1.89.0"
(https://github.com/bitcoin/bitcoin/pull/33428#pullrequestreview-3460291898)
Concept ACK on updating to the upcoming 1.90 instead of 1.89. The newer version allows dropping `skip_compiled_targets.patch`: https://github.com/hebasto/bitcoin/commit/b39ff621195238793b2ab3618291e99096fe04d3.
(https://github.com/bitcoin/bitcoin/pull/33428#pullrequestreview-3460291898)
Concept ACK on updating to the upcoming 1.90 instead of 1.89. The newer version allows dropping `skip_compiled_targets.patch`: https://github.com/hebasto/bitcoin/commit/b39ff621195238793b2ab3618291e99096fe04d3.
💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523866680)
> Did you actually call the full kernel function here, including:
Yeah, these pass for me too. And I can't find any documentation that any of this would be UB or lead to errors. Hence my confusion about how you got your error message.
```cpp
fs::path path{fs::absolute(fs::PathFromString({nullptr, 0}))};
fs::create_directories(path);
path = fs::absolute(fs::PathFromString(""));
fs::create_directories(path);
```
> I wonder what the valid meaning is here, of passing an
...
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523866680)
> Did you actually call the full kernel function here, including:
Yeah, these pass for me too. And I can't find any documentation that any of this would be UB or lead to errors. Hence my confusion about how you got your error message.
```cpp
fs::path path{fs::absolute(fs::PathFromString({nullptr, 0}))};
fs::create_directories(path);
path = fs::absolute(fs::PathFromString(""));
fs::create_directories(path);
```
> I wonder what the valid meaning is here, of passing an
...
💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523872088)
> In what situation would data_dir == nullptr and yet data_dir_len does not equal zero?
This is an invalid state and would clearly be a logic/programmer error by the caller, hence the check to catch it and return early instead of continuing.
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523872088)
> In what situation would data_dir == nullptr and yet data_dir_len does not equal zero?
This is an invalid state and would clearly be a logic/programmer error by the caller, hence the check to catch it and return early instead of continuing.
💬 maflcko commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523891180)
> Yeah, these pass for me too. And I can't find any documentation that any of this would be UB or lead to errors.
Ah, I see you are using libc++, which shows a different behavior:
https://godbolt.org/z/97aer9jde
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523891180)
> Yeah, these pass for me too. And I can't find any documentation that any of this would be UB or lead to errors.
Ah, I see you are using libc++, which shows a different behavior:
https://godbolt.org/z/97aer9jde
💬 stickies-v commented on pull request "kernel: add btck_block_tree_entry_equals":
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2523909255)
> coupling the API to Core's current internal architecture.
`CBlockIndex` is very much a kernel/validation class, not a node class (which I assume is what you mean with Core?). We're also not coupling the API to any internals, the API has no knowledge of the pointer comparison.
> b) Simpler reasoning - equality semantics are clear without needing much further context.
I feel like the current implementation is simple enough and very consistent with how validation works internally, but ye
...
(https://github.com/bitcoin/bitcoin/pull/33855#discussion_r2523909255)
> coupling the API to Core's current internal architecture.
`CBlockIndex` is very much a kernel/validation class, not a node class (which I assume is what you mean with Core?). We're also not coupling the API to any internals, the API has no knowledge of the pointer comparison.
> b) Simpler reasoning - equality semantics are clear without needing much further context.
I feel like the current implementation is simple enough and very consistent with how validation works internally, but ye
...