💬 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,
...

💬 MarcoFalke commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632931622)
Interesting. Right now, it is trivial to undo the XOR in python with something like:

```py
def util_xor(data):
with open("xor.key", "rb") as xor_f:
key = deser_string(xor_f)
for i in range(len(data)):
data[i] ^= key[i % len(key)]
return bytes(data)
```

If it requires users to instead implement a PRNG in python, they (who want to read the dat files for whatever reason) would most likely disable the XOR featur
...

💬 jamesob commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632935482)
If the motivating reason for this feature is to prevent systematized, spurious detection of things in blocks (whether via antivirus software or human), isn't it sufficient to keep the XOR key very simple? Especially, as Marco says, anything more sophisticated would complicate consuming blockfiles from external software.