💬 hebasto commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644335078)
> Can someone compile with `-save-temps` and get me the sha256_sse4.s file?

For the master branch:
- https://github.com/hebasto/artefacts/blob/master/issue-28111/asm-master/sha256_sse4.s

Master branch + the [patch](https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644167053):
- https://github.com/hebasto/artefacts/blob/master/issue-28111/asm-patched/sha256_sse4.s

💬 achow101 commented on pull request "Wallet: estimate the size of signed inputs using descriptors":
(https://github.com/bitcoin/bitcoin/pull/26567#discussion_r1269784399)
In ecdee0c3f802c6308c358a5ee501d2e031c8dcf2 "miniscript: make GetStackSize independent of P2WSH context"

The comment above this function is outdated.

💬 stratospher commented on pull request "rpc: Add test-only RPC getaddrmaninfo for new/tried table address count":
(https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1269792567)
"RPC is for testing only" was used since this is a hidden RPC.
> I think it would also be nice if documentation gave a little hint of what the new and tried are tables so the functionality is discoverable.

liked the ChatGPT documentation! will update the PR to use some of this.

💬 theuni commented on pull request "ci: document that -Wreturn-type has been fixed upstream (mingw-w64)":
(https://github.com/bitcoin/bitcoin/pull/28092#issuecomment-1644357788)
Makes sense to me. It seems a bit premature maybe, but I agree with the goal of not carrying this type of baggage over to CMake.

👍 MarcoFalke approved a pull request: "test: Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1539812584)
lgtm ACK 068b523ee7720e6a392efc294ff89bf936a3e2ac 🍈

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 068b523ee7720e6
...

💬 sipa commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644379013)
Ok so what seems to be happening is that in your build, `%16` gets mapped to `(%rsp)` (aka the memory address stored in the stack pointer), which in a few places gets used as e.g. `8+%16`, which results in `8+(%rsp)`, and that appears to be invalid asm code.

On my system (and presumably ~every build we have since this code was introduced), `%16` is mapped to `16(%rsp)` (16 bytes further than the memory address stored in the stack pointer), and thus `8+%16` becomes `8+16(%rsp)`, which is 24 by
...

💬 hebasto commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644385542)
> So... can you try if that (substitute `+%16` everywhere with `+0%16`) works?

It compiles. Going to run tests.

💬 hebasto commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644420239)
The `crypto_tests` test suit passes as well.

To ensure that the SSE4 implementation is being tested, `ENABLE_X86_SHANI`, `ENABLE_SSE41` and `ENABLE_AVX2` were disabled manually, that was confirmed in `debug.log`:
```
2023-07-20T18:38:31Z Using the 'sse4(1way)' SHA256 implementation
```

💬 hebasto commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644428684)
@sipa
> Ok so what seems to be happening is that in your build, `%16` gets mapped to `(%rsp)` (aka the memory address stored in the stack pointer), which in a few places gets used as e.g. `8+%16`, which results in `8+(%rsp)`, and that appears to be invalid asm code.
>
> On my system (and presumably ~every build we have since this code was introduced), `%16` is mapped to `16(%rsp)` (16 bytes further than the memory address stored in the stack pointer), and thus `8+%16` becomes `8+16(%rsp)`, w
...

💬 sipa commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644430606)
Yes, indirectly. Having that flag or not affects the stack layout. The issue is triggered by the "xfer" variable being allocated at the top of the stack.

📝 jonatack opened a pull request: "test: update tool_wallet coverage for unexpected writes to wallet"
(https://github.com/bitcoin/bitcoin/pull/28116)
PR #15687 added test coverage in `test/functional/tool_wallet.py` to reproduce unexpected writes to the wallet file, as described in issue https://github.com/bitcoin/bitcoin/issues/15608:

- wallet tool info unexpectedly writes to the wallet file if the wallet file permissions are read/write
- wallet tool info raises if the wallet file permissions are read-only

Update these tests/docs for two changes since #15687:

- the CI migration away from Appveyor
- the addition of descriptor walle
...

💬 MarcoFalke commented on pull request "test: update tool_wallet coverage for unexpected writes to wallet":
(https://github.com/bitcoin/bitcoin/pull/28116#discussion_r1269857579)
Instead of putting the error message in a static comment, it may be better to check that the error message is thrown at runtime, and only keep the comment that it shouldn't throw.

💬 MarcoFalke commented on pull request "refactor: Open file in FileCommit":
(https://github.com/bitcoin/bitcoin/pull/28006#issuecomment-1644469037)
Closing for now.

✅ MarcoFalke closed a pull request: "refactor: Open file in FileCommit"
(https://github.com/bitcoin/bitcoin/pull/28006)

🤔 jonatack reviewed a pull request: "test: Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850#pullrequestreview-1539942275)
ACK 068b523ee7720e6a392efc294ff89bf936a3e2ac reviewed the tests only, didn't review the CI and `is_unix_root` commits

💬 jonatack commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269874331)
It looks like the PR description section about `skip_if_root` is now out of date.

💬 jonatack commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269876108)
If you retouch, I think the logging for the assertions inside this `is_unix_root` conditional should be logged as `info` (maybe all the debug logging in this file currently could be info).

💬 sipa commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644484394)
FWIW, I assume this issue has existed since this code was introduced, but isn't harmful apart from preventing compilation, as far as I can tell.

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269884246)
thank you., fixed

💬 fanquake commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644486605)
I don't actually think it even prevents compilation, and only warns? Unclear from the issue.