💬 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_r1837269742)
All ones (binary) is not endian-sensitive. IMO it's okay for the tests to look slightly different to reduce this kind of noise.

💬 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_r1837268749)
Dummy

💬 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_r1837270655)
I want to test the setup we have in production, these tests were very useful in revealing endianness problems.

💬 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_r1837275100)
Feel like I'm imitating ChatGPT:
Ah, yes, I get now how the `memcpy()` interaction with endianness preserves the functioning of this specific test.

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