💬 theuni commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1644163497)
I don't see any reason not to do the iwyu trimming. Though sadly it doesn't look like it'll help too much. Preprocessed (gcc -E -O0) numbers for `wallet/wallet.cpp` before and after @MarcoFalke's suggested removals:
```
Before: 191,084 lines
After: 190,899 lines
```
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1644163497)
I don't see any reason not to do the iwyu trimming. Though sadly it doesn't look like it'll help too much. Preprocessed (gcc -E -O0) numbers for `wallet/wallet.cpp` before and after @MarcoFalke's suggested removals:
```
Before: 191,084 lines
After: 190,899 lines
```
💬 sipa commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644167053)
Try this:
```diff
--- a/src/crypto/sha256_sse4.cpp
+++ b/src/crypto/sha256_sse4.cpp
@@ -949,7 +949,7 @@ void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks)
"Ldone_hash_%=:"
- : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c), "=r"(d), /* e = chunk */ "=r"(f), "=r"(g), "=r"(h), "=r"(y0), "=r"(y1), "=r"(y2), "=r"(tbl), "+m"(inp_end), "+m"(inp), "+m"(xfer)
+ : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c), "=r"(d
...
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644167053)
Try this:
```diff
--- a/src/crypto/sha256_sse4.cpp
+++ b/src/crypto/sha256_sse4.cpp
@@ -949,7 +949,7 @@ void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks)
"Ldone_hash_%=:"
- : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c), "=r"(d), /* e = chunk */ "=r"(f), "=r"(g), "=r"(h), "=r"(y0), "=r"(y1), "=r"(y2), "=r"(tbl), "+m"(inp_end), "+m"(inp), "+m"(xfer)
+ : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c), "=r"(d
...
💬 sipa commented on issue "build: Windows debug cross-build fails with `-O0`":
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1644175785)
There is no requirement that an entire class is defined within a single compilation unit, I believe. If you don't have function definitions inside the `class` itself, then those definitions can be given in any .cpp file.
That said, for understandability I'd strongly prefer not splitting a class over multiple .cpp files...
(https://github.com/bitcoin/bitcoin/issues/28109#issuecomment-1644175785)
There is no requirement that an entire class is defined within a single compilation unit, I believe. If you don't have function definitions inside the `class` itself, then those definitions can be given in any .cpp file.
That said, for understandability I'd strongly prefer not splitting a class over multiple .cpp files...
💬 pinheadmz commented on pull request "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs":
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1269666789)
Great suggestion AJ this are looks a lot better now
(https://github.com/bitcoin/bitcoin/pull/27231#discussion_r1269666789)
Great suggestion AJ this are looks a lot better now
👍 jamesob approved a pull request: "test: Add missing `set -ex` to ci/lint/06_script.sh"
(https://github.com/bitcoin/bitcoin/pull/28103#pullrequestreview-1539605053)
ACK ffff4b5dc57c32bf759b705530c1368de4aa787e ([`jamesob/ackr/28103.1.MarcoFalke.test_add_missing_set_ex`](https://github.com/jamesob/bitcoin/tree/ackr/28103.1.MarcoFalke.test_add_missing_set_ex))
Tested locally; lint errors surfaced through the lint container give non-zero error code as expected, which will allow local git hooks to e.g. stop pushes on error.
(https://github.com/bitcoin/bitcoin/pull/28103#pullrequestreview-1539605053)
ACK ffff4b5dc57c32bf759b705530c1368de4aa787e ([`jamesob/ackr/28103.1.MarcoFalke.test_add_missing_set_ex`](https://github.com/jamesob/bitcoin/tree/ackr/28103.1.MarcoFalke.test_add_missing_set_ex))
Tested locally; lint errors surfaced through the lint container give non-zero error code as expected, which will allow local git hooks to e.g. stop pushes on error.
🚀 fanquake merged a pull request: "doc: correct Fedora systemtap dep"
(https://github.com/bitcoin/bitcoin/pull/28110)
(https://github.com/bitcoin/bitcoin/pull/28110)
👍 pinheadmz approved a pull request: "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs"
(https://github.com/bitcoin/bitcoin/pull/27231#pullrequestreview-1539612095)
re-ACK 93b387fe70174d174b840016da8531ef65ec313e
Confirmed changes were compiler warning fix, better test comment, and very nice clean-up in common.cpp. Built branch and ran tests locally.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 93b387fe70174d174b840016da8531ef65ec313e
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmS5WPgACgkQ5+KYS2KJ
yTp7mxAAwSLI2Fqz8+sRsHF7c/wzERlqZm7Ma8tg9ob0yyvD7Q0t89rCtc
...
(https://github.com/bitcoin/bitcoin/pull/27231#pullrequestreview-1539612095)
re-ACK 93b387fe70174d174b840016da8531ef65ec313e
Confirmed changes were compiler warning fix, better test comment, and very nice clean-up in common.cpp. Built branch and ran tests locally.
<details><summary>Show Signature</summary>
```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 93b387fe70174d174b840016da8531ef65ec313e
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmS5WPgACgkQ5+KYS2KJ
yTp7mxAAwSLI2Fqz8+sRsHF7c/wzERlqZm7Ma8tg9ob0yyvD7Q0t89rCtc
...
💬 darosior commented on issue "fuzz: miniscript: test the node is satisfiable before dereferencing `GetOps`":
(https://github.com/bitcoin/bitcoin/issues/28114#issuecomment-1644188461)
Yeah i need to investigate why it didn't trigger in the PR. Maybe i'm just wrong, i wrote this on a rush to not forget about it.
-------- Original Message --------
On Jul 20, 2023, 5:36 PM, MacrabFalke wrote:
> Heh, if there's a known bug that should be found by fuzzers, but isn't, then the tests should be fixed to find it :)
>
> —
> Reply to this email directly, [view it on GitHub](https://github.com/bitcoin/bitcoin/issues/28114#issuecomment-1644149971), or [unsubscribe](https://github.com/no
...
(https://github.com/bitcoin/bitcoin/issues/28114#issuecomment-1644188461)
Yeah i need to investigate why it didn't trigger in the PR. Maybe i'm just wrong, i wrote this on a rush to not forget about it.
-------- Original Message --------
On Jul 20, 2023, 5:36 PM, MacrabFalke wrote:
> Heh, if there's a known bug that should be found by fuzzers, but isn't, then the tests should be fixed to find it :)
>
> —
> Reply to this email directly, [view it on GitHub](https://github.com/bitcoin/bitcoin/issues/28114#issuecomment-1644149971), or [unsubscribe](https://github.com/no
...
💬 fanquake commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644194489)
> Try this:
> (replace "+m" with "+o" at the end of the line)
The same warnings occur with that diff:
```bash
# git diff
diff --git a/src/crypto/sha256_sse4.cpp b/src/crypto/sha256_sse4.cpp
index f4557291c..ebc99aa64 100644
--- a/src/crypto/sha256_sse4.cpp
+++ b/src/crypto/sha256_sse4.cpp
@@ -949,7 +949,7 @@ void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks)
"Ldone_hash_%=:"
- : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c),
...
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644194489)
> Try this:
> (replace "+m" with "+o" at the end of the line)
The same warnings occur with that diff:
```bash
# git diff
diff --git a/src/crypto/sha256_sse4.cpp b/src/crypto/sha256_sse4.cpp
index f4557291c..ebc99aa64 100644
--- a/src/crypto/sha256_sse4.cpp
+++ b/src/crypto/sha256_sse4.cpp
@@ -949,7 +949,7 @@ void Transform(uint32_t* s, const unsigned char* chunk, size_t blocks)
"Ldone_hash_%=:"
- : "+r"(s), "+r"(chunk), "+r"(blocks), "=r"(a), "=r"(b), "=r"(c),
...
🚀 fanquake merged a pull request: "test: Add missing `set -ex` to ci/lint/06_script.sh"
(https://github.com/bitcoin/bitcoin/pull/28103)
(https://github.com/bitcoin/bitcoin/pull/28103)
💬 furszy commented on issue "migratewallet crashes (wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wallet::LegacyScriptPubKeyMan::MigrateToDescriptor(): Assertion `IsMine(desc_spk) != ISMINE_NO' failed.)":
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1644205890)
@MarcoFalke, could you please try this again on master now?
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1644205890)
@MarcoFalke, could you please try this again on master now?
💬 MarcoFalke commented on pull request "refactor: Make more transaction size variables `int32_t`":
(https://github.com/bitcoin/bitcoin/pull/28059#discussion_r1269686354)
No it is not. this is still int32_t
(https://github.com/bitcoin/bitcoin/pull/28059#discussion_r1269686354)
No it is not. this is still int32_t
💬 jonatack commented on pull request "rfc: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1644206979)
If helpful, a similar pros/cons discussion took place in March 2021 starting from https://github.com/bitcoin/bitcoin/pull/21328#issuecomment-789650071.
(https://github.com/bitcoin/bitcoin/pull/28107#issuecomment-1644206979)
If helpful, a similar pros/cons discussion took place in March 2021 starting from https://github.com/bitcoin/bitcoin/pull/21328#issuecomment-789650071.
💬 MarcoFalke commented on pull request "util: Remove DirIsWritable, GetUniquePath":
(https://github.com/bitcoin/bitcoin/pull/28075#issuecomment-1644223545)
rebased
(https://github.com/bitcoin/bitcoin/pull/28075#issuecomment-1644223545)
rebased
💬 sipa commented on issue ""missing operand" assembler warnings on Windows":
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644240564)
Can someone compile with `-save-temps` and get me the sha256_sse4.s file?
(https://github.com/bitcoin/bitcoin/issues/28111#issuecomment-1644240564)
Can someone compile with `-save-temps` and get me the sha256_sse4.s file?
👍 MarcoFalke approved a pull request: "kernel: Remove UniValue from kernel library"
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1539670029)
lgtm. Left two nits, feel free to ignore.
lgtm ACK 03e06e589a24f0bcb7dbd28a0aaa925b18b87b9d 🚆
<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+krxU1A3Yux4bpwZNLvVBKy0wL
...
(https://github.com/bitcoin/bitcoin/pull/28113#pullrequestreview-1539670029)
lgtm. Left two nits, feel free to ignore.
lgtm ACK 03e06e589a24f0bcb7dbd28a0aaa925b18b87b9d 🚆
<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+krxU1A3Yux4bpwZNLvVBKy0wL
...
💬 MarcoFalke commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269708017)
Missing self-include?
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269708017)
Missing self-include?
💬 MarcoFalke commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269712862)
it may be nice to include univalue.h and then mark it as `export` for iwyu. This would allow include-sites to drop the univalue.h include, since it should be obvious that there is a univalue dependency with the `univalue_helpers.h` include already.
Also, this shouldn't increase compile cost, because it would be rare (in this case) for a call-site to include this header, but not need the univalue header.
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269712862)
it may be nice to include univalue.h and then mark it as `export` for iwyu. This would allow include-sites to drop the univalue.h include, since it should be obvious that there is a univalue dependency with the `univalue_helpers.h` include already.
Also, this shouldn't increase compile cost, because it would be rare (in this case) for a call-site to include this header, but not need the univalue header.
💬 mzumsande commented on pull request "test: fix intermittent failure in wallet_resendwallettransactions.py":
(https://github.com/bitcoin/bitcoin/pull/28108#issuecomment-1644266248)
I didn't get to address the suggestions by @jonatack and @MarcoFalke in time - they make sense to me and could be done in a separate PR that reworks the test a bit deeper than just fixing the timeout - just noting that I'm probably not going to get to this, at least for the next couple of weeks.
(https://github.com/bitcoin/bitcoin/pull/28108#issuecomment-1644266248)
I didn't get to address the suggestions by @jonatack and @MarcoFalke in time - they make sense to me and could be done in a separate PR that reworks the test a bit deeper than just fixing the timeout - just noting that I'm probably not going to get to this, at least for the next couple of weeks.
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269737484)
So I just tried this and you don't need to generate *any* additional blocks. A single call to `self.generateblock(self.nodes[1], output=f"raw(61{'ff'*998000})", transactions=[])` is enough.
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269737484)
So I just tried this and you don't need to generate *any* additional blocks. A single call to `self.generateblock(self.nodes[1], output=f"raw(61{'ff'*998000})", transactions=[])` is enough.