💬 maflcko commented on pull request "depends: Fix CMake-generated `libzmq.pc` file":
(https://github.com/bitcoin/bitcoin/pull/30508#issuecomment-2301509656)
Could rebase the upstream pull, which currently fails CI, to pick up the upstream CI fixes?

💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2301546811)
Aha.. was assuming Guix builds weren't producing debug symbols.. do the non-debug archives under */output/* contain the release-binaries? Results after untaring them:

```
$ ls -al guix-build-c3fa29db1b05/output/x86_64-linux-gnu/bitcoin-c3fa29db1b05/bin/bitcoind
-rwxr-xr-x 1 hodlinator users 16509032 jan 1 1980 guix-build-c3fa29db1b05/output/x86_64-linux-gnu/bitcoin-c3fa29db1b05/bin/bitcoind

$ ls -al guix-build-5fdbc8b4ee60/output/x86_64-linux-gnu/bitcoin-5fdbc8b4ee60/bin/bitcoind
-r
...

📝 brunoerg opened a pull request: "fuzz: speed up addrman"
(https://github.com/bitcoin/bitcoin/pull/30688)
There is no need to iterate 10'000 times and
call `Add` 10'000 times at once (we only add
1000 addresses at once in practice). These values
were introduced in 214d9055acdd72189a2f415477ce472ca8db4191.

I see an avg of 40 exec/s on master for this target and 150 exec/s on this branch.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724756834)
`CheckIsEmpty()` seems a bit more future-proof. We'll check
- no requests for this peer
- no pending orphan resolutions for this peer
- no orphans for this peer
etc.

💬 hebasto commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301622285)
I don't think that enabling CET for GCC alone will work. Shouldn't `binutils` and `glibc` also be CET-enabled?

💬 maflcko commented on pull request "fuzz: speed up addrman":
(https://github.com/bitcoin/bitcoin/pull/30688#issuecomment-2301624135)
> we only add
> 1000 addresses at once in practice

Are you referring to `MAX_ADDR_TO_SEND`? If yes, it could make sense to clarify this in the description or even in a code comemnent, given that the variable name seems to indicate it is only used for sending, not for processing/adding.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1724766037)
I called it `HaveMoreWork` since its purpose is to tell `ThreadMessageHandler()` if there is more work to do (doesn't matter what kind of work):
https://github.com/bitcoin/bitcoin/blob/3c0b66c2ece7695ab59f466d7b4d81a4c18ffd76/src/net_processing.cpp#L5083
https://github.com/bitcoin/bitcoin/blob/3c0b66c2ece7695ab59f466d7b4d81a4c18ffd76/src/net.cpp#L2902-L2903

`GetTxToReconsider`'s user is peerman, in `ProcessOrphanTx`.

I think scheduled reconsideration work could become more generic in th
...

💬 hebasto commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2301629827)
> I'd rather build up from from something like #30438 (and have a branch with similar changes).

Is that branch publicly available?

💬 fanquake commented on pull request "guix: (explicitly) build Linux GCC with `--enable-cet`":
(https://github.com/bitcoin/bitcoin/pull/30438#issuecomment-2301637021)
> will work.

What do you mean by "work" here? This PR is just explicitly turning on one option in the compiler.

> Shouldn't binutils ... also be CET-enabled?

What is a CET-enbled binutils? If you mean it supporting CET functionality, then it is new enough.

> glibc also be CET-enabled?

Last time I looked, glibc will autodetect this based on the compiler (we could still enable it explictly, but it's not clear that is required).

💬 cryptoquick commented on issue "LevelDB read failure: Corruption: block checksum mismatch":
(https://github.com/bitcoin/bitcoin/issues/30159#issuecomment-2301642234)
I'm getting this error locally also, upon IBD:

`bitcoind -server -txindex=1 -datadir=/mnt/Node/bitcoind-mainnet -rpccookiefile=/mnt/Node/bitcoind-mainnet/.cookie`

```
2024-08-21T05:15:10Z UpdateTip: new best=00000000000000000002e0f442b3299c1e3a3c56f6a4efdb063c43f0091bd949 height=822516 version=0x26e2a000 log2_work=94.618928 tx=940818791 date='2023-12-23T04:55:07Z' progress=0.872532 cache=667.5MiB(5165648txo)
2024-08-21T05:15:10Z UpdateTip: new best=00000000000000000003d85e0ab75b6831241c5
...

💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724676476)
nit: No longer needed forward declaration:
```diff
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -28,7 +28,6 @@
class arith_uint256;
class CFeeRate;
class Chainstate;
-class FastRandomContext;

/** This is connected to the logger. Can be used to redirect logs to any other log */
extern const std::function<void(const std::string&)> G_TEST_LOG_FUN;
```

💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724749568)
`rand_cache` is dead code since 16329224e70d0525208f6b0ba00c5e1531a4f5ea. Maybe remove it in an early clean-up commit?

💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724763552)
I understand this was the behavior in the past too, but should we do something else with `m_rng` if the function was called with `SeedRand::ZEROS`?

💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724773667)
Data-members should be private by default:
```diff
--- a/src/test/orphanage_tests.cpp
+++ b/src/test/orphanage_tests.cpp
@@ -20,6 +20,8 @@ BOOST_FIXTURE_TEST_SUITE(orphanage_tests, TestingSetup)

class TxOrphanageTest : public TxOrphanage
{
+ FastRandomContext& m_rng;
+
public:
TxOrphanageTest(FastRandomContext& rng) : m_rng{rng} {}

@@ -36,8 +38,6 @@ public:
it = m_orphans.begin();
return it->second.tx;
}
-
- FastRandomContext& m_rng;
...

💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724777213)
nit: No longer needed:
```diff
--- a/src/test/util/random.h
+++ b/src/test/util/random.h
@@ -7,9 +7,6 @@

#include <consensus/amount.h>
#include <random.h>
-#include <uint256.h>
-
-#include <cstdint>

enum class SeedRand {
ZEROS, //!< Seed with a compile time constant of zeros
```

💬 hodlinator commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724661382)
nit: No longer requires **test/util/random.h** and doesn't get it indirectly through **setup_common.h** since this is a benchmark:
```diff
-#include <test/util/random.h>
+#include <random.h>
```

📝 hebasto converted_to_draft a pull request: "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled"
(https://github.com/bitcoin/bitcoin/pull/30685)
CET is Intel’s [Control-flow Enforcement Technology](https://www.intel.com/content/www/us/en/developer/articles/technical/technical-look-control-flow-enforcement-technology.html).

The current GCC implementation of the `-fcf-protection` option is [based](https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fcf-protection) on CET for `x86_64-linux-gnu`.

However, on the master branch @ d79ea809d28197b1b4e3748aa1715272b53601d0, the release binaries are not marked as CET-enable
...

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724794173)
> I understand this was the behavior in the past too, but should we do something else with `m_rng` if the function was called with `SeedRand::ZEROS`?

In theory one could reseed with a constant, but I don't see the benefits, given that it:

* Requires more code to handle both cases
* Is not a refactor

So maybe this can be done in a follow-up if relevant?

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#discussion_r1724800311)
> Data-members should be private by default:

Not sure in tests, because if an object of `TxOrphanageTest` exists, it can be possibly useful to have access to the objects.m_rng for mocking. Whereas there are very little downsides (apart from being possibly confusing to use the objects.m_rng instead of the "global" m_rng).

I'll leave this as-is for now, but I may change it later, because either way is fine and I don't think it matters much.

💬 maflcko commented on pull request "test: [refactor] Use m_rng directly":
(https://github.com/bitcoin/bitcoin/pull/30571#issuecomment-2301683678)
Thanks for checking the includes in the tests @hodlinator! I think I'll leave this as-is for now, because having leftover unused includes in tests should be harmless, apart from possibly being confusing, or needing more compile time. I have a follow-up to use `iwyu` (and enforce it), so that reviewers can use the re-review cycles to clean up includes for reviewing the code itself. However, this will be a follow-up, so I'll leave this pull request as-is for now.