💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1837279112)
How about
```suggestion
// Help optimizers along by sending constant parameter values into the inlined function,
// resulting in more efficient substitutions of memcpy() -> native pow-2 copy instructions.
switch (write.size()) {
case 0: break;
case 1: XorInt(write, key, 1); break;
case 2: XorInt(write, key, 2); break;
case 4: XorInt(write, key, 4); break;
default: XorInt(write, key, write.size());
```

💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1837281249)
[My bad](https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1837275100).

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1837293159)
I ended up with only `// Help the compiler specialize 1, 2 and 4 byte cases`, since the rest was just speculation

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1837293203)
Now I have to rename the PR :D

💬 l0rinc commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1837293258)
Done

💬 gmart7t2 commented on issue "importdescriptors always rescans":
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-2469280887)
Even when the earliest timestamp specified is a week in the future it still rescans blocks from 2 hours ago.

That goes against what the documentation string says.

💬 hodlinator commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2469297214)
> > Care to explain the scaling_factor value?
>
> The values in the histogram (i.e. total bytes streamed through xor) add up to ~92gb, but 1 byte values occupy half of that. I had to scale down the histogram such that the lower values, having a few hundred occurrences aren't flattened out completely - to make the histogram still representative. Do you have a better idea?

So `raw_histogram` really is the raw data?

Added:
```C++
uint64_t raw_total_bytes{0};
for (size_t i{0}; i
...

💬 achow101 commented on issue "importdescriptors always rescans":
(https://github.com/bitcoin/bitcoin/issues/31263#issuecomment-2469315807)
It doesn't make sense for a user to import descriptors with a future timestamp. If the timestamp is greater than "now", then the timestamp for rescanning will be "now".

💬 fjahr commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1837356425)
What do you mean by "accurate"? Is the concern that someone scales this by <0.5 and then the test might fail?

📝 Talej opened a pull request: "doc: corrected listunspent rpc quoting"
(https://github.com/bitcoin/bitcoin/pull/31275)
bitcoin-cli help lockunspent RPC example shows:

```
As a JSON-RPC call
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "lockunspent", "params": [false, "[{\"txid\":\"a08e6907dbbd3d809776dbfc5d82e371b764ed838b5655e72f463568df1aadf0\",\"vout\":1}]"]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/
```
Including quotes around the transactions argument. Using the RPC in this way fails:

```
curl --data-binary '{"jsonrpc": "1.0", "id": "curltest
...

👋 Talej's pull request is ready for review: "doc: corrected listunspent rpc quoting"
(https://github.com/bitcoin/bitcoin/pull/31275)

👍 rkrux approved a pull request: "test: Add combinerawtransaction test to rpc_createmultisig"
(https://github.com/bitcoin/bitcoin/pull/31249#pullrequestreview-2428766522)
tACK 83fab3212c91d91fc5502f940c901a07772ff747

Make and functional tests successful.

Thanks for ensuring coverage for this RPC is not lost.

💬 rkrux commented on pull request "test: Add combinerawtransaction test to rpc_createmultisig":
(https://github.com/bitcoin/bitcoin/pull/31249#discussion_r1837603923)
Lacking before this change as well, can we add a comment here just like there is one in all the code blocks above and below?

`sign the rawtx with nsigs-1 signatures, then sign with the last sig, and then combine to create fully signed tx`

💬 rkrux commented on pull request "test: Add combinerawtransaction test to rpc_createmultisig":
(https://github.com/bitcoin/bitcoin/pull/31249#discussion_r1837609543)
For readability
```suggestion
full_signed_tx = node2.combinerawtransaction([rawtx2["hex"], rawtx3["hex"]])
```

💬 rkrux commented on pull request "test: Add combinerawtransaction test to rpc_createmultisig":
(https://github.com/bitcoin/bitcoin/pull/31249#discussion_r1837607106)
Although not a new change and not incorrect as well, but I noted that the last signature is from the `priv_keys[-1]`, and not `priv_keys[nsigs]`. Maybe the author intended to test signatures from non-consecutive keys as well.

🤔 hodlinator reviewed a pull request: "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2429009065)
Reviewed 57caa965b5ae284e501f892415d60fcb536f4c0e

`git range-diff master a123297 57caa96`

- Applied most of my nits that were still valid.

I understand you were burned by endianness but I disagree that it's worth sacrificing readability where endianness is a non-issue.

For cases where endianness is a concern, I suggest the following pattern:
```C++
uint64_t xor_key;
constexpr std::array<uint8_t, sizeof xor_key> xor_pat{std::to_array<uint8_t>({0xff, 0x00, 0xff, 0x00, 0xff, 0x00, 0x
...

🚀 fanquake merged a pull request: "doc: correct typos"
(https://github.com/bitcoin/bitcoin/pull/31271)

💬 fanquake commented on pull request "doc: corrected lockunspent rpc quoting":
(https://github.com/bitcoin/bitcoin/pull/31275#issuecomment-2470082122)
The same kind of quoting is used in multiple places in this file, so if it is incorrect / needs updating, all instances should be changed at the same time.

💬 fanquake commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2470110678)
I haven't really looked at the changes here, but just looking at the diff (+96'000 lines), my feedback would be that you'll need to change approach in regards to making your data available (if the intent is to have that included), as I doubt we'll be adding 96'000 lines to `bench/xor.cpp`. You could look at how we generate a header from bench/data/block413567.raw for a different approach, as including a small binary blob, and parsing it into a header at compile time is far more palatable.

💬 naumenkogs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#issuecomment-2470187172)
ACK 0ca1e05d8bcbc42892156c15f128a5f829e9e48e

Spent a fair amount of time staring at `ConsensusScriptChecks` behavior change mentioned in the original post. It seems alright. Handling various limits is probably the most complex part of this code change.

Otherwise the code seems clear and nothing is broken. Still hard for me to reason about high-level design decisions on cluster mempool here, but I learned a lot while reviewing this pr at least.