💬 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
...
🤔 janb84 reviewed a pull request: "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`"
(https://github.com/bitcoin/bitcoin/pull/33824#pullrequestreview-3460307847)
ACK fae83611b8ef358ea7aca7070fd7e82dc06f9755
The PR changes CI Tasks to use the dev-mode by default and disable options that are not applicable. This to enable kernel options from libbitcoinkernel
I like the approach of "enabling everything" and then turn of options explicitly , in this way the CI will build all the options without a lot of maintenance.
I have compared the resulting config with the old configuration , looks OK 2 non blocking questions:
ARM32 diff is only kernel stuff
...
(https://github.com/bitcoin/bitcoin/pull/33824#pullrequestreview-3460307847)
ACK fae83611b8ef358ea7aca7070fd7e82dc06f9755
The PR changes CI Tasks to use the dev-mode by default and disable options that are not applicable. This to enable kernel options from libbitcoinkernel
I like the approach of "enabling everything" and then turn of options explicitly , in this way the CI will build all the options without a lot of maintenance.
I have compared the resulting config with the old configuration , looks OK 2 non blocking questions:
ARM32 diff is only kernel stuff
...
💬 janb84 commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2523905099)
how about lines 208-212? it's windows so not sure if there is a dev-mode preset ?
```sh
- job-type: standard
generate-options: '-DBUILD_GUI=ON -DWITH_ZMQ=ON -DBUILD_BENCH=ON -DBUILD_KERNEL_LIB=ON -DBUILD_UTIL_CHAINSTATE=ON -DWERROR=ON'
job-name: 'Windows native, VS 2022'
- job-type: fuzz
generate-options: '-DVCPKG_MANIFEST_NO_DEFAULT_FEATURES=ON -DVCPKG_MANIFEST_FEATURES="wallet" -DBUILD_GUI=OFF -DBUILD_FOR_FUZZING=ON -DWERROR=ON'
...
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2523905099)
how about lines 208-212? it's windows so not sure if there is a dev-mode preset ?
```sh
- job-type: standard
generate-options: '-DBUILD_GUI=ON -DWITH_ZMQ=ON -DBUILD_BENCH=ON -DBUILD_KERNEL_LIB=ON -DBUILD_UTIL_CHAINSTATE=ON -DWERROR=ON'
job-name: 'Windows native, VS 2022'
- job-type: fuzz
generate-options: '-DVCPKG_MANIFEST_NO_DEFAULT_FEATURES=ON -DVCPKG_MANIFEST_FEATURES="wallet" -DBUILD_GUI=OFF -DBUILD_FOR_FUZZING=ON -DWERROR=ON'
...
💬 janb84 commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2523871348)
Do we still use this ? could not find something that uses this in the repo
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2523871348)
Do we still use this ? could not find something that uses this in the repo
💬 l0rinc commented on pull request "refactor: Let CCoinsViewCache::BatchWrite return void":
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2523917297)
For reference, this is what a pure virtual `CCoinsView` would look like: [l0rinc/bitcoin@`d4bec8d` (#53)](https://github.com/l0rinc/bitcoin/pull/53/commits/d4bec8ded7a75dac9aead3730e73e92f656d6e18)
A lot cleaner, but needs further work and is outside the scope of this change.
(https://github.com/bitcoin/bitcoin/pull/33866#discussion_r2523917297)
For reference, this is what a pure virtual `CCoinsView` would look like: [l0rinc/bitcoin@`d4bec8d` (#53)](https://github.com/l0rinc/bitcoin/pull/53/commits/d4bec8ded7a75dac9aead3730e73e92f656d6e18)
A lot cleaner, but needs further work and is outside the scope of this change.
💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2523928262)
> Is my understanding correct that we want to remove useless jitter that doesn't actually increase code coverage but confuses the fuzzer, such as the internal Random in leveldb's skiplist?
It depends on the specific fuzz engine as far as _exactly_ what happens (e.g. how it prioritizes or drops executed inputs). Speaking generally, if the fuzzer sees increased coverage (due to non-determinisim) it could prioritize a no-coverage-gain input and if it sees decreased coverage it could de-prioritiz
...
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2523928262)
> Is my understanding correct that we want to remove useless jitter that doesn't actually increase code coverage but confuses the fuzzer, such as the internal Random in leveldb's skiplist?
It depends on the specific fuzz engine as far as _exactly_ what happens (e.g. how it prioritizes or drops executed inputs). Speaking generally, if the fuzzer sees increased coverage (due to non-determinisim) it could prioritize a no-coverage-gain input and if it sees decreased coverage it could de-prioritiz
...
💬 maflcko commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523930442)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90299#c3:
> And for the avoidance of doubt, the problem is not that !exists(p) (although that is true) but that an empty path doesn't refer to any file system location. absolute("does not exist") is not an error, but absolute("") is.
> There is no absolute representation of the empty path, it's a category error.
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523930442)
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90299#c3:
> And for the avoidance of doubt, the problem is not that !exists(p) (although that is true) but that an empty path doesn't refer to any file system location. absolute("does not exist") is not an error, but absolute("") is.
> There is no absolute representation of the empty path, it's a category error.