👍 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.
💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269738696)
yeah, ive got this too, will push in a few minutes
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269738696)
yeah, ive got this too, will push in a few minutes
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269740640)
Any reason for this node?
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269740640)
Any reason for this node?
💬 MarcoFalke commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269743318)
My understanding is that inlining should be fine here, because if the previous nonroot code was removed, the call would fail with "path not found", does it not?
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269743318)
My understanding is that inlining should be fine here, because if the previous nonroot code was removed, the call would fail with "path not found", does it not?
💬 MarcoFalke 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-1644292192)
Sure:
```
2023-07-20T16:40:23Z [main] [init/common.cpp:153] [LogPackageVersion] Bitcoin Core version v25.99.0-d23fda05842b (release build)
2023-07-20T16:40:56Z [qt-rpcconsole] [wallet/wallet.h:895] [WalletLogPrintf] [default wallet] Migrating wallet storage database from BerkeleyDB to SQLite.
2023-07-20T17:10:25Z [scheduler] [net.cpp:1523] [DumpAddresses] [net] Flushed 0 addresses to peers.dat 56ms
bitcoin-qt: wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wa
...
(https://github.com/bitcoin/bitcoin/issues/28057#issuecomment-1644292192)
Sure:
```
2023-07-20T16:40:23Z [main] [init/common.cpp:153] [LogPackageVersion] Bitcoin Core version v25.99.0-d23fda05842b (release build)
2023-07-20T16:40:56Z [qt-rpcconsole] [wallet/wallet.h:895] [WalletLogPrintf] [default wallet] Migrating wallet storage database from BerkeleyDB to SQLite.
2023-07-20T17:10:25Z [scheduler] [net.cpp:1523] [DumpAddresses] [net] Flushed 0 addresses to peers.dat 56ms
bitcoin-qt: wallet/scriptpubkeyman.cpp:1915: std::optional<MigrationData> wa
...
💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269749581)
nope leftover from last iteration sorry, removing
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1269749581)
nope leftover from last iteration sorry, removing
💬 stickies-v commented on pull request "kernel: Remove UniValue from kernel library":
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269745657)
I don't really find this fn to be a `UniValue` helper, pretty much all the logic is sighash specific. While thinking for a better place to sit, I ended up thinking the better solution may be to keep it where it is but just remove the (imo unnecessary) dependency on UV? Offloading the default value and type check to a helper function like `ParseSighashString` is opaque, and even though my approach here is slightly more verbose in the callsite I find the clarity of what's happening to be worth it
...
(https://github.com/bitcoin/bitcoin/pull/28113#discussion_r1269745657)
I don't really find this fn to be a `UniValue` helper, pretty much all the logic is sighash specific. While thinking for a better place to sit, I ended up thinking the better solution may be to keep it where it is but just remove the (imo unnecessary) dependency on UV? Offloading the default value and type check to a helper function like `ParseSighashString` is opaque, and even though my approach here is slightly more verbose in the callsite I find the clarity of what's happening to be worth it
...