💬 l0rinc commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086632854)
we would reapply the xor on restart in that case, since we wouldn't detect that we've processed it already.
Alternatively we could keep a counter of processed blocks - modify the `xor.dat` to make sure we can't start `bitcoind` (or @LarryRuane's lock would probably also do the same) and add a height of how many blocks we've processed so far (though that would make parallelization more difficult). When we're done we'd restore the `xor.dat` file (which we have to modify anyway if we're xor-ing)
...
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086632854)
we would reapply the xor on restart in that case, since we wouldn't detect that we've processed it already.
Alternatively we could keep a counter of processed blocks - modify the `xor.dat` to make sure we can't start `bitcoind` (or @LarryRuane's lock would probably also do the same) and add a height of how many blocks we've processed so far (though that would make parallelization more difficult). When we're done we'd restore the `xor.dat` file (which we have to modify anyway if we're xor-ing)
...
💬 l0rinc commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086624487)
would it help if we used `.par_iter()` here instead (not sure if that would require adding a new lib or not, but seems like a naturally parallelizable task)?
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086624487)
would it help if we used `.par_iter()` here instead (not sure if that would require adding a new lib or not, but seems like a naturally parallelizable task)?
💬 l0rinc commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086608673)
xor is an implementation detail - I'd make it more user friendly and call it obfuscation instead
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086608673)
xor is an implementation detail - I'd make it more user friendly and call it obfuscation instead
💬 l0rinc commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086626017)
We're regularly loading blocks into memory - if we want any performance out of this, we definitely want that in my experience
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086626017)
We're regularly loading blocks into memory - if we want any performance out of this, we definitely want that in my experience
💬 fanquake commented on pull request "doc: improve NODE_NETWORK_LIMITED documentation per BIP159":
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2876206872)
What is the status of this?
(https://github.com/bitcoin/bitcoin/pull/31805#issuecomment-2876206872)
What is the status of this?
💬 fanquake commented on pull request "[28.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/32299#issuecomment-2876216158)
> was left out of the release notes
Fixed that up.
(https://github.com/bitcoin/bitcoin/pull/32299#issuecomment-2876216158)
> was left out of the release notes
Fixed that up.
💬 l0rinc commented on pull request "doc: document workaround and fallback for macOS fuzzing":
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2086646426)
Mac did trick me a few times into thinking the code was ok :/
Should I just close the PR?
(https://github.com/bitcoin/bitcoin/pull/32084#discussion_r2086646426)
Mac did trick me a few times into thinking the code was ok :/
Should I just close the PR?
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086650095)
Oh, it's actually very easy to get 8B of non-secure entropy and this is sufficient for masking: `std::hash::DefaultHasher::new().finish().to_ne_bytes()` So it can cut the dependency entirely.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086650095)
Oh, it's actually very easy to get 8B of non-secure entropy and this is sufficient for masking: `std::hash::DefaultHasher::new().finish().to_ne_bytes()` So it can cut the dependency entirely.
👍 willcl-ark approved a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32299#pullrequestreview-2836475325)
ACK 812e6375213e883a0c22aa926ebcde5da4d23a3e
Only changes to release note since previous ACK.
(https://github.com/bitcoin/bitcoin/pull/32299#pullrequestreview-2836475325)
ACK 812e6375213e883a0c22aa926ebcde5da4d23a3e
Only changes to release note since previous ACK.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086653142)
Requires the `rayon` dependency. But I strongly suspect the entire thing is bottlenecked on IO.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086653142)
Requires the `rayon` dependency. But I strongly suspect the entire thing is bottlenecked on IO.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086658185)
Oh, I see, that's clever! But then yes, this code tries to be atomic but it isn't because it doesn't call `sync_data`.
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086658185)
Oh, I see, that's clever! But then yes, this code tries to be atomic but it isn't because it doesn't call `sync_data`.
💬 Kixunil commented on pull request "contrib: add xor-blocks tool to obfuscate blocks directory":
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086662922)
Could also do it with like 4096B blocks at the time. I don't see how it would be slower since that's what `read` has to do anyway. (Though perhaps using `BufReader` would be better.)
(https://github.com/bitcoin/bitcoin/pull/32451#discussion_r2086662922)
Could also do it with like 4096B blocks at the time. I don't see how it would be slower since that's what `read` has to do anyway. (Though perhaps using `BufReader` would be better.)
👍 theStack approved a pull request: "signet: omit commitment for some trivial challenges"
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2836450095)
Code-review ACK 6adec89e3961508875e79240a19bc981e5addf1a
Left a few non-blocking nits below. I was wondering whether OP_2...OP_16 should also be treated as trivial, i.e. extend the OP_TRUE check to a range (0x81-0x96)? On one hand I can't imagine why anyone would want to use those (and even if, they could always do it via the one byte longer `0102`...`0110` instead), on the other hand one could just include them for consistency reasons.
(https://github.com/bitcoin/bitcoin/pull/29032#pullrequestreview-2836450095)
Code-review ACK 6adec89e3961508875e79240a19bc981e5addf1a
Left a few non-blocking nits below. I was wondering whether OP_2...OP_16 should also be treated as trivial, i.e. extend the OP_TRUE check to a range (0x81-0x96)? On one hand I can't imagine why anyone would want to use those (and even if, they could always do it via the one byte longer `0102`...`0110` instead), on the other hand one could just include them for consistency reasons.
💬 theStack commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2086650587)
nit: could enforce `sign` as keyword argument in order to enhance readability at the call-sites below
```suggestion
def mine_block_manual(self, node, *, sign):
```
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2086650587)
nit: could enforce `sign` as keyword argument in order to enhance readability at the call-sites below
```suggestion
def mine_block_manual(self, node, *, sign):
```
💬 theStack commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2086634429)
nit: could import and use the `OP_TRUE` constant from test_framework.script to avoid the magic number
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2086634429)
nit: could import and use the `OP_TRUE` constant from test_framework.script to avoid the magic number
💬 theStack commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2086653095)
extra-space-nit:
```suggestion
For custom signets with a trivial challenge such as `OP_TRUE` the walletprocesspsbt step can be skipped.
```
(https://github.com/bitcoin/bitcoin/pull/29032#discussion_r2086653095)
extra-space-nit:
```suggestion
For custom signets with a trivial challenge such as `OP_TRUE` the walletprocesspsbt step can be skipped.
```
💬 Kixunil commented on pull request "policy: uncap datacarrier by default":
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086699379)
> If you did indeed have 83 OP_RETURN outputs, then a datacarriersize=83 limit would mean that you'd spent your entire datacarrier budget on that overhead, without actually including any data.
Yes, but isn't it the entire point of the parameter? "to not flood the chain with garbage"
I personally don't care about it, just trying to prevent the brigaders claiming "Core snuck in a backdoor that can blow up the data size".
(https://github.com/bitcoin/bitcoin/pull/32406#discussion_r2086699379)
> If you did indeed have 83 OP_RETURN outputs, then a datacarriersize=83 limit would mean that you'd spent your entire datacarrier budget on that overhead, without actually including any data.
Yes, but isn't it the entire point of the parameter? "to not flood the chain with garbage"
I personally don't care about it, just trying to prevent the brigaders claiming "Core snuck in a backdoor that can blow up the data size".
🤔 Sjors reviewed a pull request: "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner"
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2836498591)
Concept ACK
Mostly happy with 31e3808df9c59d36a07cad07df89ae1bf9e63000
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2836498591)
Concept ACK
Mostly happy with 31e3808df9c59d36a07cad07df89ae1bf9e63000
💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086664914)
In d3f4e64d416aa29d4448dffb4d47e52997b40271 "interfaces: refactor: move `submitSolution` implementation to miner": maybe say "insert or replace"
It's unlikely for `vtx` to be empty, since we we add a coinbase during template creation.
More importantly, it's possible to call `submitSolution()` multiple times, with each call updating the coinbase. This could happen as a race condition or because the first solution was somehow incorrect.
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086664914)
In d3f4e64d416aa29d4448dffb4d47e52997b40271 "interfaces: refactor: move `submitSolution` implementation to miner": maybe say "insert or replace"
It's unlikely for `vtx` to be empty, since we we add a coinbase during template creation.
More importantly, it's possible to call `submitSolution()` multiple times, with each call updating the coinbase. This could happen as a race condition or because the first solution was somehow incorrect.
💬 Sjors commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086672679)
In 497996c3e572c8992d34e54fd13d92984e9ec21c "interfaces: refactor: move `waitNext` implementation to miner" nit, a little less wide:
```diff
diff --git a/src/node/miner.h b/src/node/miner.h
index e20cab0d08..04b12f065e 100644
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -235,9 +235,17 @@ void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& opti
/* Compute the block's merkle root, insert the coinbase transaction and the merkle root into the block */
void Add
...
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2086672679)
In 497996c3e572c8992d34e54fd13d92984e9ec21c "interfaces: refactor: move `waitNext` implementation to miner" nit, a little less wide:
```diff
diff --git a/src/node/miner.h b/src/node/miner.h
index e20cab0d08..04b12f065e 100644
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -235,9 +235,17 @@ void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& opti
/* Compute the block's merkle root, insert the coinbase transaction and the merkle root into the block */
void Add
...