💬 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.
(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.
(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)
```
(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);
```
(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
(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
...
(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
...
(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
...
(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
...
(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)
(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,
...
(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,
...
(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
...
(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.
(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.
💬 jonatack commented on pull request "util: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization":
(https://github.com/bitcoin/bitcoin/pull/28012#discussion_r1261497511)
Why this change? It is currently unused, "I need this for some stuff" is vague, and this change is being cited in https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1257088164 as a reason not to use rand test helpers consistently.
(https://github.com/bitcoin/bitcoin/pull/28012#discussion_r1261497511)
Why this change? It is currently unused, "I need this for some stuff" is vague, and this change is being cited in https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1257088164 as a reason not to use rand test helpers consistently.
💬 jonatack commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1261505790)
> The reason why I left the comment is that `randbytes` is a template function
Hm, it became a template function only a few hours before the time of the comment, due to the merge of #28012.
> It would be good to mention at least one benefit.
Consistent use of the rand test helpers for developer quality-of-life might be a benefit.
#28012 was merged without the template actually being used or any clear reason for the change.
What would be your suggestion regarding how to proceed h
...
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1261505790)
> The reason why I left the comment is that `randbytes` is a template function
Hm, it became a template function only a few hours before the time of the comment, due to the merge of #28012.
> It would be good to mention at least one benefit.
Consistent use of the rand test helpers for developer quality-of-life might be a benefit.
#28012 was merged without the template actually being used or any clear reason for the change.
What would be your suggestion regarding how to proceed h
...
💬 MarcoFalke commented on pull request "util: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization":
(https://github.com/bitcoin/bitcoin/pull/28012#discussion_r1261507530)
`std::byte` is used in many places to pass bytes. Adding this helper makes it easier to fill a vector with `std::byte`s. For example, it is used in 9999a49b3299bd25dde4805f5c68adef3876057f (https://github.com/bitcoin/bitcoin/pull/28060). But the helper can also be used in other places.
(https://github.com/bitcoin/bitcoin/pull/28012#discussion_r1261507530)
`std::byte` is used in many places to pass bytes. Adding this helper makes it easier to fill a vector with `std::byte`s. For example, it is used in 9999a49b3299bd25dde4805f5c68adef3876057f (https://github.com/bitcoin/bitcoin/pull/28060). But the helper can also be used in other places.
💬 jonatack commented on pull request "util: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization":
(https://github.com/bitcoin/bitcoin/pull/28012#discussion_r1261511420)
Thank you for clarifying for me.
(https://github.com/bitcoin/bitcoin/pull/28012#discussion_r1261511420)
Thank you for clarifying for me.
💬 jonatack commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1261513559)
Seems best to drop the last two commits here. Implementing the 3rd suggestion above might make the diff much larger and seems out of scope for this pull.
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1261513559)
Seems best to drop the last two commits here. Implementing the 3rd suggestion above might make the diff much larger and seems out of scope for this pull.
💬 sipa commented on pull request "util: Teach AutoFile how to XOR":
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632959304)
It's fair that this is perhaps not a real concern right now; I don't know. I find it slightly unsatisfying that we can't break any 8-byte repetitive pattern (see e.g. the picture [here](https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation#Electronic_codebook_(ECB)) for a visual illustration), while computationally speaking doing so should not be any concern.
Regarding external software... I'm not sure. It wouldn't surprise me that literally everyone who is interested in reading these
...
(https://github.com/bitcoin/bitcoin/pull/28060#issuecomment-1632959304)
It's fair that this is perhaps not a real concern right now; I don't know. I find it slightly unsatisfying that we can't break any 8-byte repetitive pattern (see e.g. the picture [here](https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation#Electronic_codebook_(ECB)) for a visual illustration), while computationally speaking doing so should not be any concern.
Regarding external software... I'm not sure. It wouldn't surprise me that literally everyone who is interested in reading these
...