π¬ maflcko commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2450166919)
In any case https://github.com/bitcoin/bitcoin/pull/31191 should fix the recently introduced `g_fuzzing` regression. If there are suggestions for other (more fundamental) changes or alternative code patches, separate pull requests would probably be best going forward. I am happy to review any of them, if they exist.
  (https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2450166919)
In any case https://github.com/bitcoin/bitcoin/pull/31191 should fix the recently introduced `g_fuzzing` regression. If there are suggestions for other (more fundamental) changes or alternative code patches, separate pull requests would probably be best going forward. I am happy to review any of them, if they exist.
π¬ andrewtoth commented on issue "(Past issue) On Windows, pruned nodes could crash while deleting a block file":
(https://github.com/bitcoin/bitcoin/issues/31193#issuecomment-2450172504)
Reason for reporting is that a user on the same system with only read access to the bitcoind datadir could cause bitcoind to crash. This could be seen as a form of privilege escalation. I thought it was low, but still reported in case there was something I was missing.
  (https://github.com/bitcoin/bitcoin/issues/31193#issuecomment-2450172504)
Reason for reporting is that a user on the same system with only read access to the bitcoind datadir could cause bitcoind to crash. This could be seen as a form of privilege escalation. I thought it was low, but still reported in case there was something I was missing.
π¬ hebasto commented on issue "Remove libevent as a dependency (HTTP / cli / torcontrol)":
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2450190667)
> * libevent hasn't had a release since 2020
Moreover, the new non-released code breaks our fuzzing tests. See: https://github.com/bitcoin/bitcoin/issues/30096.
  (https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2450190667)
> * libevent hasn't had a release since 2020
Moreover, the new non-released code breaks our fuzzing tests. See: https://github.com/bitcoin/bitcoin/issues/30096.
β οΈ darosior opened an issue: "(Past issue) The alert system enabled a disk-filling attack"
(https://github.com/bitcoin/bitcoin/issues/31195)
This is about a low severity issue impacting a feature removed for a decade. I can't find any public discussion about it so i'm just opening an issue to leave a trace.
This was reported on 2013-03-01 by @SergioDemianLerner that the alert system could fill-up the disk with "signature failed" messages.
That's all the information i could recollect about this old issue.
  (https://github.com/bitcoin/bitcoin/issues/31195)
This is about a low severity issue impacting a feature removed for a decade. I can't find any public discussion about it so i'm just opening an issue to leave a trace.
This was reported on 2013-03-01 by @SergioDemianLerner that the alert system could fill-up the disk with "signature failed" messages.
That's all the information i could recollect about this old issue.
β
 darosior closed an issue: "(Past issue) The alert system enabled a disk-filling attack"
(https://github.com/bitcoin/bitcoin/issues/31195)
  (https://github.com/bitcoin/bitcoin/issues/31195)
π¬ ryanofsky commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2450213536)
After testing this, it does seem like, in practice, the slowdown comes from checking the `g_fuzzing` variable, not from evaluating the assume condition. I don't agree with (or don't understand) Marco's logic that this was neccesarily the case, since before #31093 if the compiler knew evaluating the assume condition didn't have side effects and that result of the assume condition was not used, it would have no reason to evaluate it before #31093, but would need to evaluate it after #31093, so an
...
  (https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2450213536)
After testing this, it does seem like, in practice, the slowdown comes from checking the `g_fuzzing` variable, not from evaluating the assume condition. I don't agree with (or don't understand) Marco's logic that this was neccesarily the case, since before #31093 if the compiler knew evaluating the assume condition didn't have side effects and that result of the assume condition was not used, it would have no reason to evaluate it before #31093, but would need to evaluate it after #31093, so an
...
π¬ fanquake commented on issue "Remove libevent as a dependency (HTTP / cli / torcontrol)":
(https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2450216192)
> Moreover, the new non-released code breaks our fuzzing tests. See: https://github.com/bitcoin/bitcoin/issues/30096.
I'm not so sure that's relevant. I think the conclusion in that issue was that vcpkg should probably stop shipping unstable/unreleased code, and I don't think we care about fuzzing unreleased versions of dependencies. It's less suprising that code which is still in development / might have API or other breaking changes we haven't accounted for might not work correctly.
  (https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2450216192)
> Moreover, the new non-released code breaks our fuzzing tests. See: https://github.com/bitcoin/bitcoin/issues/30096.
I'm not so sure that's relevant. I think the conclusion in that issue was that vcpkg should probably stop shipping unstable/unreleased code, and I don't think we care about fuzzing unreleased versions of dependencies. It's less suprising that code which is still in development / might have API or other breaking changes we haven't accounted for might not work correctly.
π¬ marcofleon commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2450223371)
Tested ACK fafbf8acf419d5e2ca307e5804099361ca7471af
  (https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2450223371)
Tested ACK fafbf8acf419d5e2ca307e5804099361ca7471af
π¬ jb55 commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2450235448)
zfs-2.2.6-1
sata connected
zfs pool is not complicated, just a single drive (4tb ssd)
ryzen cpu `AMD Ryzen 7 1800X Eight-Core Processor`
I ran smartctl no issues, this is a brand new SSD because I thought it was my HDD.
```
=== START OF INFORMATION SECTION ===
Device Model: SPCC Solid State Disk
Serial Number: 230707575120019
LU WWN Device Id: 0 000000 000000000
Firmware Version: VE1R9007
User Capacity: 4,000,787,030,016 bytes [4.00 TB]
Sector Size: 512 bytes logic
...
  (https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2450235448)
zfs-2.2.6-1
sata connected
zfs pool is not complicated, just a single drive (4tb ssd)
ryzen cpu `AMD Ryzen 7 1800X Eight-Core Processor`
I ran smartctl no issues, this is a brand new SSD because I thought it was my HDD.
```
=== START OF INFORMATION SECTION ===
Device Model: SPCC Solid State Disk
Serial Number: 230707575120019
LU WWN Device Id: 0 000000 000000000
Firmware Version: VE1R9007
User Capacity: 4,000,787,030,016 bytes [4.00 TB]
Sector Size: 512 bytes logic
...
π¬ hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824712214)
My point about moving the negation and changing the name made more sense in the context of keeping it inside the `if`-block. If you are open to moving it out, I'd say it's better to keep the original `key_exists` name and original negation to avoid the churn and make it easier to review.
(Realized another reason for not having it inside the `if`-block is that we are mutating `obfuscate_key_vector`, which is used after the block).
  (https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824712214)
My point about moving the negation and changing the name made more sense in the context of keeping it inside the `if`-block. If you are open to moving it out, I'd say it's better to keep the original `key_exists` name and original negation to avoid the churn and make it easier to review.
(Realized another reason for not having it inside the `if`-block is that we are mutating `obfuscate_key_vector`, which is used after the block).
π€ hodlinator reviewed a pull request: "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2408330350)
(Reviewed a5cad729c76cafa047a2b1897595669ae9b2b0d5)
Since my previous Concept ACK, the PR was changed to switch the xor key more completely to `uint64_t`. Before the PR, we were already using fixed-size of 8 bytes for the obfuscation value in the file formats, so changing the type to `uint64_t` shouldn't be noticeable to users. :+1:
Even if we could move reading and XOR-ing out of the hot path as suggested by maflcko, we might as well make use the CPU architectures we have. I would expect
...
  (https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2408330350)
(Reviewed a5cad729c76cafa047a2b1897595669ae9b2b0d5)
Since my previous Concept ACK, the PR was changed to switch the xor key more completely to `uint64_t`. Before the PR, we were already using fixed-size of 8 bytes for the obfuscation value in the file formats, so changing the type to `uint64_t` shouldn't be noticeable to users. :+1:
Even if we could move reading and XOR-ing out of the hot path as suggested by maflcko, we might as well make use the CPU architectures we have. I would expect
...
π¬ hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824697175)
(Should be done in the initial commit which invalidates the comments IMO).
  (https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1824697175)
(Should be done in the initial commit which invalidates the comments IMO).
π¬ hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824750915)
> How was it addressed? I still see the same output using...
I assume you're still observing the warning for the `native_qt` package, not the `qt` package. In that case, I donβt think we need to worry about it.
  (https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824750915)
> How was it addressed? I still see the same output using...
I assume you're still observing the warning for the `native_qt` package, not the `qt` package. In that case, I donβt think we need to worry about it.
π¬ hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2450265488)
@Sjors
> I'm still confused why Ninja is needed and why nothing complains when it's missing.
>
> > Ninja is required to build the qt package in depends. It is mentioned in depends/README.md.
>
> It only mentions it in the context of an Ubuntu & Debian install.
>
> I did a depends build for [af05dd9](https://github.com/bitcoin/bitcoin/commit/af05dd9a12b89224dc7ad229698eeceb3e560ed4) again on Intel macOS 15.0.1 with Xcode 16 and no Ninja. It seems to work fine, at first glance.
>
>
...
  (https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2450265488)
@Sjors
> I'm still confused why Ninja is needed and why nothing complains when it's missing.
>
> > Ninja is required to build the qt package in depends. It is mentioned in depends/README.md.
>
> It only mentions it in the context of an Ubuntu & Debian install.
>
> I did a depends build for [af05dd9](https://github.com/bitcoin/bitcoin/commit/af05dd9a12b89224dc7ad229698eeceb3e560ed4) again on Intel macOS 15.0.1 with Xcode 16 and no Ninja. It seems to work fine, at first glance.
>
>
...
π¬ jb55 commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2450267609)
hmm I have a small dbcache size I just noticed... can't remember why I set it to that. maybe the increased fsyncs is increasing the chance of a bug somehow? can't think of anything else if its not a hardware issue.
  (https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2450267609)
hmm I have a small dbcache size I just noticed... can't remember why I set it to that. maybe the increased fsyncs is increasing the chance of a bug somehow? can't think of anything else if its not a hardware issue.
π¬ github12101 commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2450276326)
> hmm I have a small dbcache size I just noticed... can't remember why I set it to that. maybe the increased fsyncs is increasing the chance of a bug somehow? can't think of anything else if its not a hardware issue.
Can/did you run memtest86, for several hours? No errors?
  (https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2450276326)
> hmm I have a small dbcache size I just noticed... can't remember why I set it to that. maybe the increased fsyncs is increasing the chance of a bug somehow? can't think of anything else if its not a hardware issue.
Can/did you run memtest86, for several hours? No errors?
π¬ instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1824764954)
Guess I'd prefer to remove it, since it's been a slippery slope of noncompliance over time
  (https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1824764954)
Guess I'd prefer to remove it, since it's been a slippery slope of noncompliance over time
π¬ jb55 commented on issue "Fatal LevelDB error: Corruption: block checksum mismatch on Linux ext4 SATA SSDs":
(https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2450280042)
> > hmm I have a small dbcache size I just noticed... can't remember why I set it to that. maybe the increased fsyncs is increasing the chance of a bug somehow? can't think of anything else if its not a hardware issue.
>
> Can/did you run memtest86, for several hours? No errors?
I haven't done that yet because I run my entire business (https://damus.io) off this node. my system has been stable for years.
  (https://github.com/bitcoin/bitcoin/issues/30692#issuecomment-2450280042)
> > hmm I have a small dbcache size I just noticed... can't remember why I set it to that. maybe the increased fsyncs is increasing the chance of a bug somehow? can't think of anything else if its not a hardware issue.
>
> Can/did you run memtest86, for several hours? No errors?
I haven't done that yet because I run my entire business (https://damus.io) off this node. my system has been stable for years.
π¬ hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824765673)
Thanks! Updated.
  (https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1824765673)
Thanks! Updated.
π¬ darosior commented on issue "(Past issue) On Windows, pruned nodes could crash while deleting a block file":
(https://github.com/bitcoin/bitcoin/issues/31193#issuecomment-2450283360)
Of course, better safe than sorry with this matter!
  (https://github.com/bitcoin/bitcoin/issues/31193#issuecomment-2450283360)
Of course, better safe than sorry with this matter!
