💬 MarcoFalke commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1632780362)
> How much faster are they with -O2?

Yes.

(Locally the functional tests go from `(hasn't finished)` to `19769s (accumulated)`)

💬 dergoegge commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1632783352)
> (Locally the functional tests go from (hasn't finished) to 19769s (accumulated))

Any results for the fuzz task? Just to compare it to the speed up observed in #27992

💬 MarcoFalke commented on issue "valgrind: Syscall param ppoll(ufds.events) points to uninitialised byte(s)":
(https://github.com/bitcoin/bitcoin/issues/28072#issuecomment-1632784452)
Maybe someone can transform this into a standalone minimal cpp file to submit to valgrind or musl?

💬 hebasto commented on pull request "guix: Remove librt usage from release binaries":
(https://github.com/bitcoin/bitcoin/pull/28069#issuecomment-1632831214)
Guix builds:
```
67078bddd5dc32801b8c916c3bc12f1404da572312f0158a89b9603c1f753969 guix-build-8f6f0d81ee3a/output/aarch64-linux-gnu/SHA256SUMS.part
794dd00009860fd67d7e51463ee1c5ea9677dfff1c739dd0b91cf73136deb655 guix-build-8f6f0d81ee3a/output/aarch64-linux-gnu/bitcoin-8f6f0d81ee3a-aarch64-linux-gnu-debug.tar.gz
eb9cf3f472ffbc37446fe4d80fe81dc62cf1c28c4d57dd8a7b7176e65487aeeb guix-build-8f6f0d81ee3a/output/aarch64-linux-gnu/bitcoin-8f6f0d81ee3a-aarch64-linux-gnu.tar.gz
e775a9e9b23be44b5c7
...

💬 MarcoFalke commented on issue "valgrind: Syscall param ppoll(ufds.events) points to uninitialised byte(s)":
(https://github.com/bitcoin/bitcoin/issues/28072#issuecomment-1632844998)
Done:

```
/bitcoin-core # cat /tmp/a.cpp
#include <sys/types.h>
#include <sys/socket.h>
#include <netdb.h>

int main(){
addrinfo ai_hint{};
ai_hint.ai_socktype = SOCK_STREAM;
ai_hint.ai_protocol = IPPROTO_TCP;
ai_hint.ai_family = AF_UNSPEC;
ai_hint.ai_flags = true? AI_ADDRCONFIG : AI_NUMERICHOST;
addrinfo* ai_res{nullptr};
return getaddrinfo("foo.bar", nullptr, &ai_hint, &ai_res);
}
/bitcoin-core # clang++ /tmp/a.cpp -o /tmp/exe && valgrind --gen-s
...

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261403669)
Would be cleaner to write:
```suggestion
if (!Assume(tx)) {
return false;
}
```

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261410424)
(I've mentioned this offline as well)

The locking assumptions around this check are weird because `m_txpackagetracker` has its own internal mutex where as `m_txrequest` is guarded by `cs_main`. There is nothing stopping `m_txpackagetracker->Count()` form returning something different right after this check.

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261405442)
This can be dropped in favor of just checking that the CNodeState exists

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261416426)
Not a big fan of using `Assume` like this because we can't test these conditions but they **can** happen in production (if the caller is doing something wrong).

I'd suggest just returning if these assumptions don't hold.

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261414179)
Imo, it would be nicer to do the orphanage changes separately, including amended functional/unit/fuzz tests.

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261412063)
```suggestion
void PeerManagerImpl::AddOrphanAnnouncer(NodeId nodeid, const CTransactionRef& tx, std::chrono::microseconds current_time)
```

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261419680)
```suggestion
void MempoolAcceptedTx(const CTransactionRef& tx);
```

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261419537)
Would be nice to add unit and fuzz tests for this

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261436145)
```suggestion
// Add the orphan's parents. Net processing will filter out what we already have.
// Deduplicate parent txids, so that we don't have to loop over
// the same parent txid more than once down below.
std::set<uint256> unique_parents;
auto to_prevout = [](const CTxIn& in) { return in.prevout.hash; };
std::transform(ptx->vin.begin(), ptx->vin.end(), std::inserter(unique_parents, unique_parents.begin()), to_pre
...

💬 dergoegge commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1261440012)
Would you mind adding some spacing around logical blocks in your code? I find a lot of the new code hard to read with a lack of spacing

e.g.:
```suggestion
{
AssertLockNotHeld(m_mutex);
LOCK(m_mutex);

std::vector<std::pair<NodeId, GenTxid>> expired;
auto tracker_requestable = orphan_request_tracker.GetRequestable(nodeid, current_time, &expired);
for (const auto& entry : expired) {
LogPrint(BCLog::TXPACKAGES, "\nTimeout of infli
...

👍 jamesob approved a pull request: "util: Teach AutoFile how to XOR"
(https://github.com/bitcoin/bitcoin/pull/28060#pullrequestreview-1526786597)
ACK fadb32556f64a72377c82be71a207195e6fcf68b ([`jamesob/ackr/28060.2.MarcoFalke.util_add_xorfile`](https://github.com/jamesob/bitcoin/tree/ackr/28060.2.MarcoFalke.util_add_xorfile))

Looks good! More concise change now that we're just adding XOR capability to existing classes.

<details><summary>Show signature data</summary>
<p>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK fadb32556f64a72377c82be71a207195e6fcf68b ([`jamesob/ackr/28060.2.MarcoFalke.util_add_xorfile`](https
...

💬 MarcoFalke commented on pull request "ci: Add missing -O2 to valgrind tasks":
(https://github.com/bitcoin/bitcoin/pull/28071#issuecomment-1632883877)
Yes.

From:
```
Run asmap_direct with args ['valgrind', '--quiet', '--error-exitcode=1', '/root/bitcoin/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', PosixPath('/root/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/asmap_direct')]INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 98738393
INFO: Loaded 1 modules (248427 inline 8-bit counters): 248427 [0x2781858, 0x27be2c3),
INFO: Loaded 1 PC tables (248427 PCs): 248427 [0x27be2c8,0x2b8
...

🚀 achow101 merged a pull request: "Add support for RFC8439 variant of ChaCha20"
(https://github.com/bitcoin/bitcoin/pull/27985)

💬 sipa commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632907785)
(Slight tangent, not necessarily for this PR)

I was wondering if it wouldn't be better to XOR with a proper (but non-cryptographic) RNG output instead of a fixed byte pattern, as it'd avoid repetitive patterns remaining in the file, or weak keys that could accidentally be selected.

E.g. Xoshiro256++ is extremely fast (multiple GiB per second), see https://prng.di.unimi.it/. However, it's nontrivial to seek in, which would be needed here. I got a bit carried away and wrote code to do that,
...

💬 mzumsande commented on pull request "kernel: Remove StartShutdown calls from validation code":
(https://github.com/bitcoin/bitcoin/pull/28048#discussion_r1261480112)
I looked into this a bit deeply and did some test runs on signet, and now I'm no longer sure that this is a behavioral change requiring a release note in the first place:

My understanding is that the IBD behavior on master is:
* `ActivateBestChainStep()`  will connect only one block if we made progress
* The inner do / while loop of `ActivateBestChain` also will only run once unless the chain has less work than intitially (only relevant in a reorg).
* so during IBD we'll connect one block,
...