💬 ajtowns commented on pull request "txgraph: drop move assignment operator":
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3527384125)
ACK aef40b93cf057d2a27d61881b0858d491206bcd3 -- matches what I was thinking
Adding some assertions in `Ref::Ref(Ref&&)` triggers when I run the fuzz binary over some txgraph corpus data I generated previously, so https://github.com/bitcoin/bitcoin/pull/33862#discussion_r2519205585 doesn't seem like a real problem.
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3527384125)
ACK aef40b93cf057d2a27d61881b0858d491206bcd3 -- matches what I was thinking
Adding some assertions in `Ref::Ref(Ref&&)` triggers when I run the fuzz binary over some txgraph corpus data I generated previously, so https://github.com/bitcoin/bitcoin/pull/33862#discussion_r2519205585 doesn't seem like a real problem.
👍 l0rinc approved a pull request: "net_processing: rename RelayTransaction to better describe what it does"
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3459390175)
I don't want to be dismissive of @ajtowns' claims. I agree they're all valid, and please allow me to push back respectfully.
> bikeshedding about the names of functions is not a good use of anyone's time here.
I've been tricked many times by misnamed methods. If you think the old name is good, or the new name is worse, or the new comment isn't adding anything, I can empathize with that. But nobody is forcing us to review this. I personally do it because I want some progress with the origin
...
(https://github.com/bitcoin/bitcoin/pull/33565#pullrequestreview-3459390175)
I don't want to be dismissive of @ajtowns' claims. I agree they're all valid, and please allow me to push back respectfully.
> bikeshedding about the names of functions is not a good use of anyone's time here.
I've been tricked many times by misnamed methods. If you think the old name is good, or the new name is worse, or the new comment isn't adding anything, I can empathize with that. But nobody is forcing us to review this. I personally do it because I want some progress with the origin
...
💬 l0rinc commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2523145922)
the enum name aligns more closely with the method name 👍
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2523145922)
the enum name aligns more closely with the method name 👍
💬 stickies-v commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523160503)
I can't seem to reproduce that, e.g. these tests all pass for me, indicating that an empty or null path resolves to `fs::path()`, which then expands to `fs::current_path()`? How did you get that fatal error?
```cpp
BOOST_CHECK_EQUAL(fs::PathFromString(""), fs::path());
BOOST_CHECK_EQUAL(fs::PathFromString({nullptr, 0}), fs::path());
BOOST_CHECK_EQUAL(fs::canonical(fs::path()), fs::current_path());
```
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523160503)
I can't seem to reproduce that, e.g. these tests all pass for me, indicating that an empty or null path resolves to `fs::path()`, which then expands to `fs::current_path()`? How did you get that fatal error?
```cpp
BOOST_CHECK_EQUAL(fs::PathFromString(""), fs::path());
BOOST_CHECK_EQUAL(fs::PathFromString({nullptr, 0}), fs::path());
BOOST_CHECK_EQUAL(fs::canonical(fs::path()), fs::current_path());
```
💬 maflcko commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523224076)
> How did you get that fatal error?
It is hidden in the error message, but it says "cannot make absolute path".
Did you actually call the full kernel function here, including:
```cpp
fs::path abs_data_dir{fs::absolute(fs::PathFromString({data_dir, data_dir_len}))};
fs::create_directories(abs_data_dir);
```
In any case, even if it doesn't crash, I wonder what the valid meaning is here, of passing an empty string?
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523224076)
> How did you get that fatal error?
It is hidden in the error message, but it says "cannot make absolute path".
Did you actually call the full kernel function here, including:
```cpp
fs::path abs_data_dir{fs::absolute(fs::PathFromString({data_dir, data_dir_len}))};
fs::create_directories(abs_data_dir);
```
In any case, even if it doesn't crash, I wonder what the valid meaning is here, of passing an empty string?
💬 yancyribbens commented on pull request "kernel: allow null data_directory":
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523230856)
In what situation would `data_dir == nullptr` and yet `data_dir_len` does not equal zero? In other-words, if data_dir is null and data_dir_len is `1` then continue?
(https://github.com/bitcoin/bitcoin/pull/33867#discussion_r2523230856)
In what situation would `data_dir == nullptr` and yet `data_dir_len` does not equal zero? In other-words, if data_dir is null and data_dir_len is `1` then continue?
💬 ryanofsky commented on pull request "scripted-diff: fix leftover references to `policy/fees.h`":
(https://github.com/bitcoin/bitcoin/pull/33864#issuecomment-3527726460)
Thanks for the cleanup!
(https://github.com/bitcoin/bitcoin/pull/33864#issuecomment-3527726460)
Thanks for the cleanup!
💬 fanquake commented on pull request "guix: build `bitcoin-qt` with static libxcb & utils":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3527955912)
Guix Build (x86_64 & aarch64):
```bash
fe9e088e3481013bf59dfc500803b415bf29a28cb8721974040e1ea7d0a75770 guix-build-96963b888e5a/output/aarch64-linux-gnu/SHA256SUMS.part
7e070397b6bace67e0960c2e9838717c77117571eaff88aaad9e6c03d24a9400 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu-debug.tar.gz
9979dbb9896915bf99c9d23accccb5cab8f6b81de17c51d03c328763270f14f3 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu.tar.g
...
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3527955912)
Guix Build (x86_64 & aarch64):
```bash
fe9e088e3481013bf59dfc500803b415bf29a28cb8721974040e1ea7d0a75770 guix-build-96963b888e5a/output/aarch64-linux-gnu/SHA256SUMS.part
7e070397b6bace67e0960c2e9838717c77117571eaff88aaad9e6c03d24a9400 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu-debug.tar.gz
9979dbb9896915bf99c9d23accccb5cab8f6b81de17c51d03c328763270f14f3 guix-build-96963b888e5a/output/aarch64-linux-gnu/bitcoin-96963b888e5a-aarch64-linux-gnu.tar.g
...
💬 maflcko commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3527984072)
review ACK 6d2c8ea9dbd77c71051935b5ab59224487509559 🔰
<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 6d2c8ea9dbd7
...
(https://github.com/bitcoin/bitcoin/pull/33553#issuecomment-3527984072)
review ACK 6d2c8ea9dbd77c71051935b5ab59224487509559 🔰
<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 6d2c8ea9dbd7
...
✅ 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