💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1644136991)
Rebased (and corrected a comment in the fuzz test witness padding commit).
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1644136991)
Rebased (and corrected a comment in the fuzz test witness padding commit).
💬 achow101 commented on pull request "test: fix intermittent failure in wallet_resendwallettransactions.py":
(https://github.com/bitcoin/bitcoin/pull/28108#issuecomment-1644144474)
ACK e667bd68a10512ddc784df44bdcb63ee441e5551
(https://github.com/bitcoin/bitcoin/pull/28108#issuecomment-1644144474)
ACK e667bd68a10512ddc784df44bdcb63ee441e5551
💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269641610)
It is already ugly to have a timeout in the functional tests, and use it to sync them. (At least this one is std::optional)
However, my preference would still be to not have this and instead force the caller to sync.
Otherwise this will just lead to brittle code down the line and potentially intermittent test issues when running with libc++ sanitizers in valgrind on arm64, etc.
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1269641610)
It is already ugly to have a timeout in the functional tests, and use it to sync them. (At least this one is std::optional)
However, my preference would still be to not have this and instead force the caller to sync.
Otherwise this will just lead to brittle code down the line and potentially intermittent test issues when running with libc++ sanitizers in valgrind on arm64, etc.
💬 MarcoFalke commented on issue "fuzz: miniscript: test the node is satisfiable before dereferencing `GetOps`":
(https://github.com/bitcoin/bitcoin/issues/28114#issuecomment-1644149971)
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 :)
(https://github.com/bitcoin/bitcoin/issues/28114#issuecomment-1644149971)
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 :)
💬 jonatack commented on pull request "rpc: fix data optionality for RPC calls.":
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1644155681)
@miketwenty1 Needs rebase. Are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/27829#issuecomment-1644155681)
@miketwenty1 Needs rebase. Are you still working on this?
✅ achow101 closed an issue: "failure in wallet_resendwallettransactions.py --legacy-wallet"
(https://github.com/bitcoin/bitcoin/issues/28094)
(https://github.com/bitcoin/bitcoin/issues/28094)
🚀 achow101 merged a pull request: "test: fix intermittent failure in wallet_resendwallettransactions.py"
(https://github.com/bitcoin/bitcoin/pull/28108)
(https://github.com/bitcoin/bitcoin/pull/28108)
⚠️ jonatack opened an issue: "Win64 CI failure in feature_versionbits_warning.py"
(https://github.com/bitcoin/bitcoin/issues/28115)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
https://cirrus-ci.com/task/6148314429652992?logs=functional_tests#L2861
### Expected behaviour
Expect CI to pass for unrelated changes.
### Steps to reproduce
https://cirrus-ci.com/task/6148314429652992?logs=functional_tests#L2861
### Relevant log output
```
test 2023-07-19T19:45:37.140000Z TestFramework (ERROR): Assertion failed
Traceback (mos
...
(https://github.com/bitcoin/bitcoin/issues/28115)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
https://cirrus-ci.com/task/6148314429652992?logs=functional_tests#L2861
### Expected behaviour
Expect CI to pass for unrelated changes.
### Steps to reproduce
https://cirrus-ci.com/task/6148314429652992?logs=functional_tests#L2861
### Relevant log output
```
test 2023-07-19T19:45:37.140000Z TestFramework (ERROR): Assertion failed
Traceback (mos
...
💬 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