💬 furszy commented on pull request "Fix crash when closing wallet":
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2350111993)
> This seems like a workable fix, but I think it it is not ideal. The underlying problem here is that https://github.com/bitcoin/bitcoin/pull/30659 moved NotifyUnload from FlushAndDeleteWallet to RemoveWallet, so now instead of NotifyUnload guaranteed being called once, it can now be called multiple times, but the GUI code assumes it is only called once and double deletes the wallet model and crashes.
As `RemoveWallet` erases the wallet from the context's wallets vector (locking the context's
...
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2350111993)
> This seems like a workable fix, but I think it it is not ideal. The underlying problem here is that https://github.com/bitcoin/bitcoin/pull/30659 moved NotifyUnload from FlushAndDeleteWallet to RemoveWallet, so now instead of NotifyUnload guaranteed being called once, it can now be called multiple times, but the GUI code assumes it is only called once and double deletes the wallet model and crashes.
As `RemoveWallet` erases the wallet from the context's wallets vector (locking the context's
...
💬 gmaxwell commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2350134473)
@sdaftuar
Iteration counts as the metric overstates the improvements from things that reduce iteration counts but increase work per iteration, and understates the improvements from things that only make iterations faster. But I totally get why cutting off based on time would be unworkable from a practical benchmarking perspective and the differences are large enough that this is still very interesting.
I thought it was interesting that the 100k and 1m lines get slightly worse in the last
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2350134473)
@sdaftuar
Iteration counts as the metric overstates the improvements from things that reduce iteration counts but increase work per iteration, and understates the improvements from things that only make iterations faster. But I totally get why cutting off based on time would be unworkable from a practical benchmarking perspective and the differences are large enough that this is still very interesting.
I thought it was interesting that the 100k and 1m lines get slightly worse in the last
...
💬 ryanofsky commented on pull request "Fix crash when closing wallet":
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2350230453)
> As `RemoveWallet` erases the wallet from the context's wallets vector (locking the context's mutex) prior to calling `NotifyUnload`, the only way it could be called twice is if we re-add the wallet in-between the two `RemoveWallet` calls. Which shouldn't be possible anywhere.
Oh, I didn't notice there is a `return false` in the middle of `RemoveWallet` so it does look like RemoveWallet code path is safe and won't (currently) notify more than once.
Nevertheless it doesn't make sense for t
...
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2350230453)
> As `RemoveWallet` erases the wallet from the context's wallets vector (locking the context's mutex) prior to calling `NotifyUnload`, the only way it could be called twice is if we re-add the wallet in-between the two `RemoveWallet` calls. Which shouldn't be possible anywhere.
Oh, I didn't notice there is a `return false` in the middle of `RemoveWallet` so it does look like RemoveWallet code path is safe and won't (currently) notify more than once.
Nevertheless it doesn't make sense for t
...
💬 achow101 commented on pull request "kernel: Move background load thread to node context":
(https://github.com/bitcoin/bitcoin/pull/30896#issuecomment-2350367403)
ACK bc7900f33db3d01fb93dfee7981c01ea495cd42e
(https://github.com/bitcoin/bitcoin/pull/30896#issuecomment-2350367403)
ACK bc7900f33db3d01fb93dfee7981c01ea495cd42e
🚀 achow101 merged a pull request: "kernel: Move background load thread to node context"
(https://github.com/bitcoin/bitcoin/pull/30896)
(https://github.com/bitcoin/bitcoin/pull/30896)
🤔 jonatack reviewed a pull request: "refactor: move m_is_inbound out of CNodeState"
(https://github.com/bitcoin/bitcoin/pull/30233#pullrequestreview-2304335527)
Post-merge ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5
(https://github.com/bitcoin/bitcoin/pull/30233#pullrequestreview-2304335527)
Post-merge ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5
💬 jonatack commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1759630416)
FWIW I tried adding an assert here while reviewing this pull and found it unexpectedly difficult. Happy to review if someone does it.
As a sanity check verified both unit and functional tests appear to fail if this line is changed or omitted.
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1759630416)
FWIW I tried adding an assert here while reviewing this pull and found it unexpectedly difficult. Happy to review if someone does it.
As a sanity check verified both unit and functional tests appear to fail if this line is changed or omitted.
💬 sipa commented on pull request "streams: cache file position within AutoFile":
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2350782260)
@achow101 I've added a commit that gets rid of `AutoFile::Get` entirely (based on @davidgumberg's d238c46a44292d5ed81d703089b66be333a68083).
(https://github.com/bitcoin/bitcoin/pull/30884#issuecomment-2350782260)
@achow101 I've added a commit that gets rid of `AutoFile::Get` entirely (based on @davidgumberg's d238c46a44292d5ed81d703089b66be333a68083).
💬 petertodd commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1759707389)
@instagibbs What MARA told me was that their nodes do not enforce the dust limit. Notably, that means if Libre Relay or some other implementation removed the dust limit, transactions violating it would get to MARA and be mined.
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1759707389)
@instagibbs What MARA told me was that their nodes do not enforce the dust limit. Notably, that means if Libre Relay or some other implementation removed the dust limit, transactions violating it would get to MARA and be mined.
💬 ryanofsky commented on pull request "Fix crash when closing wallet":
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2350959839)
One way to test difference between the current fix and suggested fix is to delay the notification:
```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -174,7 +174,10 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet
context.wallets.erase(i);
}
// Notify unload so that upper layers release the shared pointer.
- wallet->NotifyUnload();
+ std::thread([wallet]() {
+ std::this_thread::sleep_for(std::chrono::s
...
(https://github.com/bitcoin-core/gui/pull/835#issuecomment-2350959839)
One way to test difference between the current fix and suggested fix is to delay the notification:
```diff
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -174,7 +174,10 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet
context.wallets.erase(i);
}
// Notify unload so that upper layers release the shared pointer.
- wallet->NotifyUnload();
+ std::thread([wallet]() {
+ std::this_thread::sleep_for(std::chrono::s
...
📝 hebasto opened a pull request: "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets"
(https://github.com/bitcoin/bitcoin/pull/30901)
This PR leverages the approach from the https://github.com/chaincodelabs/libmultiprocess project and introduces a new `target_data_sources()` function, which minimizes code needed to assign a `*.json` or `*.raw` data file to the `test_bitcoin` or `bench_bitcoin` target.
(https://github.com/bitcoin/bitcoin/pull/30901)
This PR leverages the approach from the https://github.com/chaincodelabs/libmultiprocess project and introduces a new `target_data_sources()` function, which minimizes code needed to assign a `*.json` or `*.raw` data file to the `test_bitcoin` or `bench_bitcoin` target.
💬 hebasto commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2350962600)
Friendly ping @ryanofsky :)
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2350962600)
Friendly ping @ryanofsky :)
💬 hebasto commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1759725111)
The comment above no longer seems correct.
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1759725111)
The comment above no longer seems correct.
📝 hebasto opened a pull request: "Remove Autotools packages from depends and CI"
(https://github.com/bitcoin/bitcoin/pull/30902)
This PR is a follow-up to https://github.com/bitcoin/bitcoin/pull/30752 and addresses https://github.com/bitcoin/bitcoin/pull/30752#discussion_r1758594864.
(https://github.com/bitcoin/bitcoin/pull/30902)
This PR is a follow-up to https://github.com/bitcoin/bitcoin/pull/30752 and addresses https://github.com/bitcoin/bitcoin/pull/30752#discussion_r1758594864.
💬 hebasto commented on pull request "guix: Drop unused autotools packages":
(https://github.com/bitcoin/bitcoin/pull/30752#discussion_r1759727460)
Thanks! Addressed in https://github.com/bitcoin/bitcoin/pull/30902.
(https://github.com/bitcoin/bitcoin/pull/30752#discussion_r1759727460)
Thanks! Addressed in https://github.com/bitcoin/bitcoin/pull/30902.
💬 fanquake commented on pull request "Remove Autotools packages from depends and CI":
(https://github.com/bitcoin/bitcoin/pull/30902#discussion_r1759727845)
I don't think this is correct. Given we still have depends packages that can output libtool archives (which we currently remove).
(https://github.com/bitcoin/bitcoin/pull/30902#discussion_r1759727845)
I don't think this is correct. Given we still have depends packages that can output libtool archives (which we currently remove).
💬 Sjors commented on pull request "ci: honor ci bypass prefix in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2350973678)
> when opening or updating a PR that only touches a markdown file, I'd use this
We might still want to run spell check and such things for documentation changes. Skipping unnecessary tasks could be done by CI itself. So I don't think that's an argument for this PR.
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2350973678)
> when opening or updating a PR that only touches a markdown file, I'd use this
We might still want to run spell check and such things for documentation changes. Skipping unnecessary tasks could be done by CI itself. So I don't think that's an argument for this PR.
💬 hebasto commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1759729804)
Adjusted in https://github.com/bitcoin/bitcoin/pull/30901.
(https://github.com/bitcoin/bitcoin/pull/30415#discussion_r1759729804)
Adjusted in https://github.com/bitcoin/bitcoin/pull/30901.
💬 hebasto commented on pull request "Remove Autotools packages from depends and CI":
(https://github.com/bitcoin/bitcoin/pull/30902#discussion_r1759730052)
Thanks! Reverted back.
(https://github.com/bitcoin/bitcoin/pull/30902#discussion_r1759730052)
Thanks! Reverted back.
💬 jonatack commented on pull request "ci: honor ci bypass prefix in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2350987707)
> > when opening or updating a PR that only touches a markdown file, I'd use this
>
> We might still want to run spell check and such things for documentation changes.
Insofar as the spelling linter in the CI doesn’t raise, running it in the CI isn't very important -- it's a facultative aid that can be run locally by authors and reviewers. Thus, I'd use it on my own pulls to avoid wasting CI cycles where they aren't needed.
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2350987707)
> > when opening or updating a PR that only touches a markdown file, I'd use this
>
> We might still want to run spell check and such things for documentation changes.
Insofar as the spelling linter in the CI doesn’t raise, running it in the CI isn't very important -- it's a facultative aid that can be run locally by authors and reviewers. Thus, I'd use it on my own pulls to avoid wasting CI cycles where they aren't needed.