💬 Sjors commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1793951200)
re-utACK b1b12d1b88492fe66b26243760689c7f040c67c2
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1793951200)
re-utACK b1b12d1b88492fe66b26243760689c7f040c67c2
👍 Sjors approved a pull request: "wallet: prevent bugs from invalid transaction heights with asserts, comments, and refactoring"
(https://github.com/bitcoin/bitcoin/pull/28546#pullrequestreview-1714193806)
utACK f06016d77d848133fc6568f287bb86b644c9fa69
(https://github.com/bitcoin/bitcoin/pull/28546#pullrequestreview-1714193806)
utACK f06016d77d848133fc6568f287bb86b644c9fa69
💬 Sjors commented on pull request "wallet: prevent bugs from invalid transaction heights with asserts, comments, and refactoring":
(https://github.com/bitcoin/bitcoin/pull/28546#discussion_r1382736556)
262a78b13365e5318741187fde431c4974539494: could expand this comment:
```
// Find block by hash and fill in the state height.
```
(https://github.com/bitcoin/bitcoin/pull/28546#discussion_r1382736556)
262a78b13365e5318741187fde431c4974539494: could expand this comment:
```
// Find block by hash and fill in the state height.
```
⚠️ Sjors opened an issue: "test: wallet_miniscript.py fails with thread sanitizer "
(https://github.com/bitcoin/bitcoin/issues/28800)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Using clang version 15.0.7 on Ubuntu and enabling the thread sanitizer, the `wallet_miniscript.py --descriptors` test consistently fails with a timeout.
### Expected behaviour
Don't timeout.
I could of course increate the timeouts, but this is the only test with that issue.
As an aside, the log output contains extremely long descriptors which should probably be truncated.
### Step
...
(https://github.com/bitcoin/bitcoin/issues/28800)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Using clang version 15.0.7 on Ubuntu and enabling the thread sanitizer, the `wallet_miniscript.py --descriptors` test consistently fails with a timeout.
### Expected behaviour
Don't timeout.
I could of course increate the timeouts, but this is the only test with that issue.
As an aside, the log output contains extremely long descriptors which should probably be truncated.
### Step
...
💬 Sjors commented on issue "test: wallet_miniscript.py fails with thread sanitizer ":
(https://github.com/bitcoin/bitcoin/issues/28800#issuecomment-1794029975)
I'll try if @theStack's #28799 helps.
(https://github.com/bitcoin/bitcoin/issues/28800#issuecomment-1794029975)
I'll try if @theStack's #28799 helps.
💬 Sjors commented on pull request "wallet: cache descriptor ID to avoid repeated descriptor string creation":
(https://github.com/bitcoin/bitcoin/pull/28799#issuecomment-1794115914)
This fixes #28800 for me and run _way_ faster under `--with-sanitizers=thread`.
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
(https://github.com/bitcoin/bitcoin/pull/28799#issuecomment-1794115914)
This fixes #28800 for me and run _way_ faster under `--with-sanitizers=thread`.
ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
⚠️ Sjors opened an issue: "p2p_segwit.py fails with thread sanitizer"
(https://github.com/bitcoin/bitcoin/issues/28801)
### Current behaviour
Using clang version 15.0.7 on Ubuntu and enabling the thread sanitizer, the `p2p_segwit.py --descriptors` test consistently fails.
It fails during the `Subtest: test_wtxid_relay (Segwit active = True)`.
It's different from #28800 in that this isn't a timeout. The subtests fails after about 5 seconds.
```
Node returned unexpected exit code (66) vs (0) when stopping
```
### Expected behaviour
Don't fail.
### Steps to reproduce
```
./autogen
./confi
...
(https://github.com/bitcoin/bitcoin/issues/28801)
### Current behaviour
Using clang version 15.0.7 on Ubuntu and enabling the thread sanitizer, the `p2p_segwit.py --descriptors` test consistently fails.
It fails during the `Subtest: test_wtxid_relay (Segwit active = True)`.
It's different from #28800 in that this isn't a timeout. The subtests fails after about 5 seconds.
```
Node returned unexpected exit code (66) vs (0) when stopping
```
### Expected behaviour
Don't fail.
### Steps to reproduce
```
./autogen
./confi
...
💬 Sjors commented on issue "p2p_segwit.py fails with thread sanitizer":
(https://github.com/bitcoin/bitcoin/issues/28801#issuecomment-1794142983)
Some warnings from the sanitiser: https://gist.github.com/Sjors/607c3426136db1c91b89acea3c62affb
(https://github.com/bitcoin/bitcoin/issues/28801#issuecomment-1794142983)
Some warnings from the sanitiser: https://gist.github.com/Sjors/607c3426136db1c91b89acea3c62affb
✅ Sjors closed an issue: "p2p_segwit.py fails with thread sanitizer"
(https://github.com/bitcoin/bitcoin/issues/28801)
(https://github.com/bitcoin/bitcoin/issues/28801)
💬 Sjors commented on issue "p2p_segwit.py fails with thread sanitizer":
(https://github.com/bitcoin/bitcoin/issues/28801#issuecomment-1794150128)
Closing this. When I first saw this failure it was while running multiple tests in parallel. That was run by a script which _does_ use supressions. But when I tried to reproduce I forgot the suppressions. Running the indidivual test with suppressions on doesn't fail. And when I ran the test suite in parallel again it also didn't fail. So I think it was just a random failure.
I'll try to get the logs from the parallel run next time.
(https://github.com/bitcoin/bitcoin/issues/28801#issuecomment-1794150128)
Closing this. When I first saw this failure it was while running multiple tests in parallel. That was run by a script which _does_ use supressions. But when I tried to reproduce I forgot the suppressions. Running the indidivual test with suppressions on doesn't fail. And when I ran the test suite in parallel again it also didn't fail. So I think it was just a random failure.
I'll try to get the logs from the parallel run next time.
💬 Daniel600 commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1794185915)
> I think it’s good to communicate on the mailing list in which version (27.0 or 28.0) full-rbf might be turn on by default. I advocated such setting in the past as early as 0.24, though at the time some 0-conf transactions applications and services were migrating their stuff in consequence.
HI @ariard - has this PR been decided to be merged already? What would the timeline to expect if it will be released with 27.0 or 28.0? (i.e When should we expect 27.0 or 28.0 to released?)
At GAP600
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1794185915)
> I think it’s good to communicate on the mailing list in which version (27.0 or 28.0) full-rbf might be turn on by default. I advocated such setting in the past as early as 0.24, though at the time some 0-conf transactions applications and services were migrating their stuff in consequence.
HI @ariard - has this PR been decided to be merged already? What would the timeline to expect if it will be released with 27.0 or 28.0? (i.e When should we expect 27.0 or 28.0 to released?)
At GAP600
...
💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1794248918)
Re https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1669359482
> What are the steps to reproduce?
Run the fuzzer on the fuzz test introduced in this PR seeded with Sjors' inputs from his [qa repo PR](https://github.com/bitcoin-core/qa-assets/pull/140) and compiled with bdb enabled.
> Outside of that, my recommendations would be to use bdb-5.3
The bugs persist even with bdb 5.3.
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1794248918)
Re https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1669359482
> What are the steps to reproduce?
Run the fuzzer on the fuzz test introduced in this PR seeded with Sjors' inputs from his [qa repo PR](https://github.com/bitcoin-core/qa-assets/pull/140) and compiled with bdb enabled.
> Outside of that, my recommendations would be to use bdb-5.3
The bugs persist even with bdb 5.3.
💬 Sjors commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1794251948)
The first commit ee79f05f1fef57bfbea24333a46e525385d02319 introduces a dangling reference warning for me. gcc 13.1.0 on Ubuntu 23.04.
```
./configure --enable-suppress-external-warnings --disable-fuzz-binary --without-gui
make
...
validation.cpp: In member function ‘PackageMempoolAcceptResult {anonymous}::MemPoolAccept::AcceptPackage(const Package&, ATMPArgs&)’:
validation.cpp:1488:36: warning: possibly dangling reference to a temporary [-Wdangling-reference]
1488 | const
...
(https://github.com/bitcoin/bitcoin/pull/28391#issuecomment-1794251948)
The first commit ee79f05f1fef57bfbea24333a46e525385d02319 introduces a dangling reference warning for me. gcc 13.1.0 on Ubuntu 23.04.
```
./configure --enable-suppress-external-warnings --disable-fuzz-binary --without-gui
make
...
validation.cpp: In member function ‘PackageMempoolAcceptResult {anonymous}::MemPoolAccept::AcceptPackage(const Package&, ATMPArgs&)’:
validation.cpp:1488:36: warning: possibly dangling reference to a temporary [-Wdangling-reference]
1488 | const
...
💬 YellowRoseCx commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1794275787)
> > I think it’s good to communicate on the mailing list in which version (27.0 or 28.0) full-rbf might be turn on by default. I advocated such setting in the past as early as 0.24, though at the time some 0-conf transactions applications and services were migrating their stuff in consequence.
>
> HI @ariard - has this PR been decided to be merged already? What would the timeline to expect if it will be released with 27.0 or 28.0? (i.e When should we expect 27.0 or 28.0 to released?)
>
> At G
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1794275787)
> > I think it’s good to communicate on the mailing list in which version (27.0 or 28.0) full-rbf might be turn on by default. I advocated such setting in the past as early as 0.24, though at the time some 0-conf transactions applications and services were migrating their stuff in consequence.
>
> HI @ariard - has this PR been decided to be merged already? What would the timeline to expect if it will be released with 27.0 or 28.0? (i.e When should we expect 27.0 or 28.0 to released?)
>
> At G
...
💬 S3RK commented on pull request "wallet: cache descriptor ID to avoid repeated descriptor string creation":
(https://github.com/bitcoin/bitcoin/pull/28799#issuecomment-1794276769)
Code Review ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
The change looks correct. I also verified that when we update descriptors with `importdescriptor` command we replace the whole `WalletDescriptor` object, so ID remains correct in that case.
(https://github.com/bitcoin/bitcoin/pull/28799#issuecomment-1794276769)
Code Review ACK 5e6bc6d830664a5afeb5d5bd7e7b3818a01376b7
The change looks correct. I also verified that when we update descriptors with `importdescriptor` command we replace the whole `WalletDescriptor` object, so ID remains correct in that case.
📝 ajtowns opened a pull request: "ArgsManager: support subcommand-specific options"
(https://github.com/bitcoin/bitcoin/pull/28802)
Adds the ability to link particular options to one or more subcommands, and uses this feature in `bitcoin-wallet`. Separates out the help information for subcommand-specific options (duplicating it if an option applies to multiple subcommands), and provides a function for checking if some options have been specified that only apply to different subcommands.
(https://github.com/bitcoin/bitcoin/pull/28802)
Adds the ability to link particular options to one or more subcommands, and uses this feature in `bitcoin-wallet`. Separates out the help information for subcommand-specific options (duplicating it if an option applies to multiple subcommands), and provides a function for checking if some options have been specified that only apply to different subcommands.
💬 ajtowns commented on pull request "ArgsManager: support subcommand-specific options":
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-1794358855)
Implements [this comment](https://github.com/bitcoin/bitcoin/pull/20715#discussion_r555574662) from when these commands were introduced originally.
(https://github.com/bitcoin/bitcoin/pull/28802#issuecomment-1794358855)
Implements [this comment](https://github.com/bitcoin/bitcoin/pull/20715#discussion_r555574662) from when these commands were introduced originally.
💬 jsarenik commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1794367265)
@ajtowns wrote:
> What's the actual problem you're trying to solve here? Would it work to have the signet miner only generate blocks when the mempool has entries, and sit idle when it's empty, for instance? If you're trying to generate many blocks, then regtest is already better as it doesn't require a baseline proof of work, and simply having multiple regtest configs probably makes more sense.
The last [comment](https://pricey.pages.dev/loglog) makes a point. It really is hard when anyone c
...
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1794367265)
@ajtowns wrote:
> What's the actual problem you're trying to solve here? Would it work to have the signet miner only generate blocks when the mempool has entries, and sit idle when it's empty, for instance? If you're trying to generate many blocks, then regtest is already better as it doesn't require a baseline proof of work, and simply having multiple regtest configs probably makes more sense.
The last [comment](https://pricey.pages.dev/loglog) makes a point. It really is hard when anyone c
...
💬 maflcko commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1794381260)
Ok, but then I think this is a problem for the qa-assets repo to solve. As I said, BDB is not enabled in the fuzz CI, so this shouldn't cause CI issues.
One way for qa-assets to solve this would be to exclude inputs that crash bdb.
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1794381260)
Ok, but then I think this is a problem for the qa-assets repo to solve. As I said, BDB is not enabled in the fuzz CI, so this shouldn't cause CI issues.
One way for qa-assets to solve this would be to exclude inputs that crash bdb.
💬 maflcko commented on issue "test: wallet_miniscript.py fails with thread sanitizer ":
(https://github.com/bitcoin/bitcoin/issues/28800#issuecomment-1794386419)
This also happens on the qemu-s390x and valgrind CI tasks
(https://github.com/bitcoin/bitcoin/issues/28800#issuecomment-1794386419)
This also happens on the qemu-s390x and valgrind CI tasks