💬 jonatack commented on pull request "ci: honor ci bypass prefix in test-each-commit":
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2349722686)
This brings back memories when contributing to Ruby on Rails, that `skip ci` was requested to be used in any doc-only changes.
No strong opinion, but when opening or updating a PR that only touches a markdown file, I'd use this.
(https://github.com/bitcoin/bitcoin/pull/30898#issuecomment-2349722686)
This brings back memories when contributing to Ruby on Rails, that `skip ci` was requested to be used in any doc-only changes.
No strong opinion, but when opening or updating a PR that only touches a markdown file, I'd use this.
💬 achow101 commented on pull request "qt: Translations update":
(https://github.com/bitcoin/bitcoin/pull/30899#issuecomment-2349757169)
ACK ae0529576147a1a5bee992574e2cefc8a1fa37d0
(https://github.com/bitcoin/bitcoin/pull/30899#issuecomment-2349757169)
ACK ae0529576147a1a5bee992574e2cefc8a1fa37d0
👍 tdb3 approved a pull request: "test: Check already deactivated network stays suspended after dumptxoutset"
(https://github.com/bitcoin/bitcoin/pull/30892#pullrequestreview-2303789215)
tested ACK 72c9a1fe94f927220d3159f516fda684ae9d4caa
Great addition.
Added improper `node.setnetworkactive()` lines above each call to `self.check_expected_network()` to ensure the checks would fail. They did (no surprise).
(https://github.com/bitcoin/bitcoin/pull/30892#pullrequestreview-2303789215)
tested ACK 72c9a1fe94f927220d3159f516fda684ae9d4caa
Great addition.
Added improper `node.setnetworkactive()` lines above each call to `self.check_expected_network()` to ensure the checks would fail. They did (no surprise).
💬 hebasto commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1759331776)
Why does referencing `../src/ipc/capnp/` not work from `src/test/CMakeLists.txt`?
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1759331776)
Why does referencing `../src/ipc/capnp/` not work from `src/test/CMakeLists.txt`?
💬 achow101 commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#issuecomment-2349919694)
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5
(https://github.com/bitcoin/bitcoin/pull/30233#issuecomment-2349919694)
ACK 07f4cebe5780f1039541d989e64b70eccc5b4eb5
🚀 achow101 merged a pull request: "refactor: move m_is_inbound out of CNodeState"
(https://github.com/bitcoin/bitcoin/pull/30233)
(https://github.com/bitcoin/bitcoin/pull/30233)
💬 sdaftuar commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2350047595)
> @sdaftuar is that number of iterations per search invocation, or total over the entire linearization?
This is the total over the entire linearization, not each search invocation.
> Could a version of this graph show the total fraction of _transactions_ that are optimally linearized?
This seems a bit difficult to state, since a transaction may be part of many clusters over its lifetime, so I'm not sure precisely what we'd want to measure.
>Or maybe just do a chart where a cluster m
...
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2350047595)
> @sdaftuar is that number of iterations per search invocation, or total over the entire linearization?
This is the total over the entire linearization, not each search invocation.
> Could a version of this graph show the total fraction of _transactions_ that are optimally linearized?
This seems a bit difficult to state, since a transaction may be part of many clusters over its lifetime, so I'm not sure precisely what we'd want to measure.
>Or maybe just do a chart where a cluster m
...
🤔 furszy reviewed a pull request: "doc: cmake: prepend "build" to functional/test_runner.py"
(https://github.com/bitcoin/bitcoin/pull/30859#pullrequestreview-2304019304)
Concept ACK. I keep calling the src test files by inertia..
(https://github.com/bitcoin/bitcoin/pull/30859#pullrequestreview-2304019304)
Concept ACK. I keep calling the src test files by inertia..
💬 achow101 commented on pull request "test: Check already deactivated network stays suspended after dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/30892#issuecomment-2350072632)
ACK 72c9a1fe94f927220d3159f516fda684ae9d4caa
(https://github.com/bitcoin/bitcoin/pull/30892#issuecomment-2350072632)
ACK 72c9a1fe94f927220d3159f516fda684ae9d4caa
🚀 achow101 merged a pull request: "test: Check already deactivated network stays suspended after dumptxoutset"
(https://github.com/bitcoin/bitcoin/pull/30892)
(https://github.com/bitcoin/bitcoin/pull/30892)
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1759454074)
I haven't taken this yet, given I'm working on a last commit reworking part of how the fanout selection works. I may consider it after that. In any case, looks like this is only filling the cache but not testing anything after it
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1759454074)
I haven't taken this yet, given I'm working on a last commit reworking part of how the fanout selection works. I may consider it after that. In any case, looks like this is only filling the cache but not testing anything after it
💬 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.