💬 rkrux commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020669952)
The naming of the `failure` variable is unintuitive imo because the first time I read it, I assumed this is some returned failure object from some operation undergoing the test but that is not the case as per this comment in the `make_spender()`.
https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b54271b0addaf4b/test/functional/feature_taproot.py#L493
At the cost of being verbose, it could be called `failure_overrides` or something similar because seeing around ~90 odd occurr
...
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020669952)
The naming of the `failure` variable is unintuitive imo because the first time I read it, I assumed this is some returned failure object from some operation undergoing the test but that is not the case as per this comment in the `make_spender()`.
https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b54271b0addaf4b/test/functional/feature_taproot.py#L493
At the cost of being verbose, it could be called `failure_overrides` or something similar because seeing around ~90 odd occurr
...
💬 rkrux commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020681949)
A note here on the `key` object could be helpful because it appears to be a commonly used arg as there are around ~75 occurrences of this object in this file. Moreover, it's not even a named argument in the `make_spender()` but a passthrough. The only mention of it is here:
https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b54271b0addaf4b/test/functional/feature_taproot.py#L425
An indirect mention is here:
https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b5427
...
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2020681949)
A note here on the `key` object could be helpful because it appears to be a commonly used arg as there are around ~75 occurrences of this object in this file. Moreover, it's not even a named argument in the `make_spender()` but a passthrough. The only mention of it is here:
https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b54271b0addaf4b/test/functional/feature_taproot.py#L425
An indirect mention is here:
https://github.com/bitcoin/bitcoin/blob/4c1906a500cacab385b09e780b5427
...
🤔 rkrux reviewed a pull request: "test: Add encodable PUSHDATA1 examples to feature_taproot"
(https://github.com/bitcoin/bitcoin/pull/32114#pullrequestreview-2728655786)
Sorry if these comments are coming in a bit late.
(https://github.com/bitcoin/bitcoin/pull/32114#pullrequestreview-2728655786)
Sorry if these comments are coming in a bit late.
✅ hebasto closed an issue: "oss-fuzz build fails"
(https://github.com/bitcoin/bitcoin/issues/32167)
(https://github.com/bitcoin/bitcoin/issues/32167)
👍 TheCharlatan approved a pull request: "validation: set BLOCK_FAILED_CHILD correctly"
(https://github.com/bitcoin/bitcoin/pull/31835#pullrequestreview-2728780393)
Re-ACK c99667583dd9b57612edf4c04611cd4857250600
(https://github.com/bitcoin/bitcoin/pull/31835#pullrequestreview-2728780393)
Re-ACK c99667583dd9b57612edf4c04611cd4857250600
💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020752757)
Thanks! I'll overload `HasLegacyRecords` so it'd make it even cleaner I think.
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020752757)
Thanks! I'll overload `HasLegacyRecords` so it'd make it even cleaner I think.
💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020753904)
Yeah, I'll tweak it a bit. Thanks!
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020753904)
Yeah, I'll tweak it a bit. Thanks!
💬 rkrux commented on pull request "wallet: migration, avoid loading legacy wallet after failure when BDB isn't compiled":
(https://github.com/bitcoin/bitcoin/pull/31451#discussion_r2020760391)
I'll let furszy confirm but I believe conceptually this^ is what he meant. You can open a PR for others to review and provide suggestions. Also, you can keep the PR in draft mode if you prefer so, which lets the reviewers know the PR is not ready for review yet.
But it's easier and better to give suggestions in a PR than it is on a standalone commit - you will also get CI feedback while you're waiting for a response.
(https://github.com/bitcoin/bitcoin/pull/31451#discussion_r2020760391)
I'll let furszy confirm but I believe conceptually this^ is what he meant. You can open a PR for others to review and provide suggestions. Also, you can keep the PR in draft mode if you prefer so, which lets the reviewers know the PR is not ready for review yet.
But it's easier and better to give suggestions in a PR than it is on a standalone commit - you will also get CI feedback while you're waiting for a response.
✅ maflcko closed a pull request: "qt: Add addressList field to SendCoinsRecipient for multiple addresses"
(https://github.com/bitcoin-core/gui/pull/857)
(https://github.com/bitcoin-core/gui/pull/857)
💬 maflcko commented on pull request "qt: Add addressList field to SendCoinsRecipient for multiple addresses":
(https://github.com/bitcoin-core/gui/pull/857#issuecomment-2765787428)
Closing for now, per previous comment. Feel free to open a fresh pull request, which passes all tests, and the CI. Also, a screenshot and steps to reproduce or a test would be good for a new feature.
(https://github.com/bitcoin-core/gui/pull/857#issuecomment-2765787428)
Closing for now, per previous comment. Feel free to open a fresh pull request, which passes all tests, and the CI. Also, a screenshot and steps to reproduce or a test would be good for a new feature.
💬 pinheadmz commented on pull request "[draft] Replace libevent with our own HTTP and socket-handling implementation":
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2765788869)
I rebased this branch on a single squashed commit from #30988 essentially just cherry-picking `sockman.{h,cpp}` by @vasild and leaving out the p2p refactor. This will make rebase maintenance on master a lot easier by reducing conflicting scope, and hopefully also makes review easier. It also means to some extent this PR can be merged independently of #30988, and also gives @theuni some room to rewrite a specific HTTP sockman if a more efficient purpose-focused module can be written. (Will updat
...
(https://github.com/bitcoin/bitcoin/pull/32061#issuecomment-2765788869)
I rebased this branch on a single squashed commit from #30988 essentially just cherry-picking `sockman.{h,cpp}` by @vasild and leaving out the p2p refactor. This will make rebase maintenance on master a lot easier by reducing conflicting scope, and hopefully also makes review easier. It also means to some extent this PR can be merged independently of #30988, and also gives @theuni some room to rewrite a specific HTTP sockman if a more efficient purpose-focused module can be written. (Will updat
...
👋 pinheadmz's pull request is ready for review: "[draft] Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061)
(https://github.com/bitcoin/bitcoin/pull/32061)
⚠️ maflcko opened an issue: "Segfault during shutdown in SendCoinsDialog::updateCoinControlState"
(https://github.com/bitcoin-core/gui/issues/862)
During IBD, while shutting down the gui, it crashed with `Thread 1 "bitcoin-qt" received signal SIGSEGV, Segmentation fault.` Version: v29.99.0-af3dee0b8d45
The `bt` is:
```
(gdb) bt
#0 0x00005555558ba7b2 in SendCoinsDialog::updateCoinControlState (
this=this@entry=0x55555833f710) at ./qt/sendcoinsdialog.cpp:850
#1 0x00005555558ba88e in SendCoinsDialog::updateSmartFeeLabel (
this=0x55555833f710) at ./qt/sendcoinsdialog.cpp:863
#2 0x0000555556a99c3e in QObject::event(QEvent*) ()
#3
...
(https://github.com/bitcoin-core/gui/issues/862)
During IBD, while shutting down the gui, it crashed with `Thread 1 "bitcoin-qt" received signal SIGSEGV, Segmentation fault.` Version: v29.99.0-af3dee0b8d45
The `bt` is:
```
(gdb) bt
#0 0x00005555558ba7b2 in SendCoinsDialog::updateCoinControlState (
this=this@entry=0x55555833f710) at ./qt/sendcoinsdialog.cpp:850
#1 0x00005555558ba88e in SendCoinsDialog::updateSmartFeeLabel (
this=0x55555833f710) at ./qt/sendcoinsdialog.cpp:863
#2 0x0000555556a99c3e in QObject::event(QEvent*) ()
#3
...
⚠️ maflcko typed an issue: "Segfault during shutdown in SendCoinsDialog::updateCoinControlState"
(https://github.com/bitcoin-core/gui/issues/862)
During IBD, while shutting down the gui, it crashed with `Thread 1 "bitcoin-qt" received signal SIGSEGV, Segmentation fault.` Version: v29.99.0-af3dee0b8d45
The `bt` is:
```
(gdb) bt
#0 0x00005555558ba7b2 in SendCoinsDialog::updateCoinControlState (
this=this@entry=0x55555833f710) at ./qt/sendcoinsdialog.cpp:850
#1 0x00005555558ba88e in SendCoinsDialog::updateSmartFeeLabel (
this=0x55555833f710) at ./qt/sendcoinsdialog.cpp:863
#2 0x0000555556a99c3e in QObject::event(QEvent*) ()
#3
...
(https://github.com/bitcoin-core/gui/issues/862)
During IBD, while shutting down the gui, it crashed with `Thread 1 "bitcoin-qt" received signal SIGSEGV, Segmentation fault.` Version: v29.99.0-af3dee0b8d45
The `bt` is:
```
(gdb) bt
#0 0x00005555558ba7b2 in SendCoinsDialog::updateCoinControlState (
this=this@entry=0x55555833f710) at ./qt/sendcoinsdialog.cpp:850
#1 0x00005555558ba88e in SendCoinsDialog::updateSmartFeeLabel (
this=0x55555833f710) at ./qt/sendcoinsdialog.cpp:863
#2 0x0000555556a99c3e in QObject::event(QEvent*) ()
#3
...
💬 pablomartin4btc commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#issuecomment-2765810531)
_<ins>Updates</ins>:_
- Addressed @fjahr's feedback.
(https://github.com/bitcoin/bitcoin/pull/32149#issuecomment-2765810531)
_<ins>Updates</ins>:_
- Addressed @fjahr's feedback.
💬 hebasto commented on issue "Segfault during shutdown in SendCoinsDialog::updateCoinControlState":
(https://github.com/bitcoin-core/gui/issues/862#issuecomment-2765819734)
cc @achow101 @furszy
(https://github.com/bitcoin-core/gui/issues/862#issuecomment-2765819734)
cc @achow101 @furszy
💬 maflcko commented on issue "Segfault during shutdown in SendCoinsDialog::updateCoinControlState":
(https://github.com/bitcoin-core/gui/issues/862#issuecomment-2765841334)
I guess this is a race from the block tip event (probably optimized out in the bt) and the wallet unloading.
(https://github.com/bitcoin-core/gui/issues/862#issuecomment-2765841334)
I guess this is a race from the block tip event (probably optimized out in the bt) and the wallet unloading.
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020819698)
> Any reason not to use bash + sed here?
Mostly because `sed` is not mentioned in the image [docs](https://github.com/actions/runner-images/blob/win22/20250324.3/images/windows/Windows2022-Readme.md).
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020819698)
> Any reason not to use bash + sed here?
Mostly because `sed` is not mentioned in the image [docs](https://github.com/actions/runner-images/blob/win22/20250324.3/images/windows/Windows2022-Readme.md).
💬 maflcko commented on issue "Segfault during shutdown in SendCoinsDialog::updateCoinControlState":
(https://github.com/bitcoin-core/gui/issues/862#issuecomment-2765876870)
Steps to reproduce:
* Apply diff on current master to create more (fake) block tip events for the gui
```diff
diff --git a/src/init.cpp b/src/init.cpp
index f35a547c92..7c5fc7f65f 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1885,7 +1885,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
int64_t best_block_time{};
{
LOCK(chainman.GetMutex());
- const auto& tip{*Assert(chainman.ActiveTip())};
+ auto& tip{*Assert(chainman.Ac
...
(https://github.com/bitcoin-core/gui/issues/862#issuecomment-2765876870)
Steps to reproduce:
* Apply diff on current master to create more (fake) block tip events for the gui
```diff
diff --git a/src/init.cpp b/src/init.cpp
index f35a547c92..7c5fc7f65f 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1885,7 +1885,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
int64_t best_block_time{};
{
LOCK(chainman.GetMutex());
- const auto& tip{*Assert(chainman.ActiveTip())};
+ auto& tip{*Assert(chainman.Ac
...
💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020837144)
Wouldn't it restore an invalidated cache when the affected files have been modified in a meaningful way?
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r2020837144)
Wouldn't it restore an invalidated cache when the affected files have been modified in a meaningful way?
💬 ajtowns commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2765898369)
> I suggest we reconsider this. To add on to the earlier points in favor of this approach, I ran into "tx-size-small" when testing new policies with packages: a 1-in-1-out child spending the p2a of a parent can be less than 64 bytes.
That's easily fixed by having the 1-out be a 0sat OP_RETURN pushing three bytes of data... I remain Concept and Approach NACK on this, supporting 60-63 byte txs but not 64 byte txs just seems like it's asking for trouble. ie, just add the extra bytes to your outp
...
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2765898369)
> I suggest we reconsider this. To add on to the earlier points in favor of this approach, I ran into "tx-size-small" when testing new policies with packages: a 1-in-1-out child spending the p2a of a parent can be less than 64 bytes.
That's easily fixed by having the 1-out be a 0sat OP_RETURN pushing three bytes of data... I remain Concept and Approach NACK on this, supporting 60-63 byte txs but not 64 byte txs just seems like it's asking for trouble. ie, just add the extra bytes to your outp
...