💬 andrewtoth commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2417406390)
```suggestion
/** Pass this transaction to node for mempool insertion and optional relay to peers. */
```
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2417406390)
```suggestion
/** Pass this transaction to node for mempool insertion and optional relay to peers. */
```
💬 andrewtoth commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2417410085)
```suggestion
* @param[in] broadcast_method whether broadcast the transaction after inserting it into the mempool
```
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2417410085)
```suggestion
* @param[in] broadcast_method whether broadcast the transaction after inserting it into the mempool
```
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417413809)
Adding a dependency from a transaction to itself is a no-op. I don't think we rely on that, but it doesn't hurt to support.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417413809)
Adding a dependency from a transaction to itself is a no-op. I don't think we rely on that, but it doesn't hurt to support.
💬 andrewtoth commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417441038)
> The transaction isn't really scheduled; its broadcast time hasn't been decided.
Hmm but isn't the announcements being queued just an implementation detail? The tx may or may not be queued for announcement to each individual peer. If they are queued, then we can say they have been scheduled for those peers at the next periodic send.
> Later, depending on what's in the priority queue and what else we receive, might inv it. "Announce" is more specific than "Broadcast" for tx relay.
Same
...
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417441038)
> The transaction isn't really scheduled; its broadcast time hasn't been decided.
Hmm but isn't the announcements being queued just an implementation detail? The tx may or may not be queued for announcement to each individual peer. If they are queued, then we can say they have been scheduled for those peers at the next periodic send.
> Later, depending on what's in the priority queue and what else we receive, might inv it. "Announce" is more specific than "Broadcast" for tx relay.
Same
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417449859)
It only modifies the `linchunking` object, which is local inside `SanityCheck()`.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417449859)
It only modifies the `linchunking` object, which is local inside `SanityCheck()`.
💬 glozow commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417451419)
> If they are queued, then we can say they have been scheduled for those peers at the next periodic send.
No, we can't. The next send will only include the highest feerate transactions in the queue. If we keep receiving higher feerate transactions, it won't be inv'd for some time.
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417451419)
> If they are queued, then we can say they have been scheduled for those peers at the next periodic send.
No, we can't. The next send will only include the highest feerate transactions in the queue. If we keep receiving higher feerate transactions, it won't be inv'd for some time.
💬 achow101 commented on pull request "Revert "depends: Update URL for `qrencode` package source tarball"":
(https://github.com/bitcoin/bitcoin/pull/33577#issuecomment-3386885492)
> Which I assume will be fixed when `qrencode-4.1.1-fukuchi.org.tar.gz` is renamed on the depends-sources cache.
It's renamed (again)
(https://github.com/bitcoin/bitcoin/pull/33577#issuecomment-3386885492)
> Which I assume will be fixed when `qrencode-4.1.1-fukuchi.org.tar.gz` is renamed on the depends-sources cache.
It's renamed (again)
💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417528472)
you're right, my mistake
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417528472)
you're right, my mistake
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417529982)
Every transaction in a cluster will end up in its linearization, so the transaction count can at most be 1 more than the largest possible linearization index. This makes it fairly appropriate as a type, I think.
DepGraphIndex could work too (and I use that elsewhere for transaction counts in a cluster), do you think that's more clear? It's a bit misleading too, because not every cluster actually uses a depgraph internally (singletons don't).
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417529982)
Every transaction in a cluster will end up in its linearization, so the transaction count can at most be 1 more than the largest possible linearization index. This makes it fairly appropriate as a type, I think.
DepGraphIndex could work too (and I use that elsewhere for transaction counts in a cluster), do you think that's more clear? It's a bit misleading too, because not every cluster actually uses a depgraph internally (singletons don't).
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417532341)
Indeed!
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417532341)
Indeed!
💬 achow101 commented on pull request "[30.x] Finalise v30.0":
(https://github.com/bitcoin/bitcoin/pull/33559#issuecomment-3386945202)
ACK d615eb6998eeccb9106854dffa95f36b319177e1
(https://github.com/bitcoin/bitcoin/pull/33559#issuecomment-3386945202)
ACK d615eb6998eeccb9106854dffa95f36b319177e1
📝 Aathish101 opened a pull request: "newly Update funcs.mk"
(https://github.com/bitcoin/bitcoin/pull/33590)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/33590)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 Aathish101 commented on pull request "newly Update funcs.mk":
(https://github.com/bitcoin/bitcoin/pull/33590#issuecomment-3386948673)
updated
(https://github.com/bitcoin/bitcoin/pull/33590#issuecomment-3386948673)
updated
💬 achow101 commented on pull request "doc: how to update a subtree":
(https://github.com/bitcoin/bitcoin/pull/33568#issuecomment-3386959793)
ACK a1226bc760c70a22ef4a197d5690aca4d83cb74c
(https://github.com/bitcoin/bitcoin/pull/33568#issuecomment-3386959793)
ACK a1226bc760c70a22ef4a197d5690aca4d83cb74c
🚀 achow101 merged a pull request: "doc: how to update a subtree"
(https://github.com/bitcoin/bitcoin/pull/33568)
(https://github.com/bitcoin/bitcoin/pull/33568)
💬 l0rinc commented on pull request "net: make m_nodes_mutex non-recursive":
(https://github.com/bitcoin/bitcoin/pull/32394#discussion_r2417567225)
there are a lot of changes in a single commit that seem not strictly related. Could you maybe extract low-risk refactorings to separate commits so that the more riskier ones are as simple as possible? I prefer the commits telling me a story for why we're doing the change, but here I see a lot of changes that aren't guiding me.
(https://github.com/bitcoin/bitcoin/pull/32394#discussion_r2417567225)
there are a lot of changes in a single commit that seem not strictly related. Could you maybe extract low-risk refactorings to separate commits so that the more riskier ones are as simple as possible? I prefer the commits telling me a story for why we're doing the change, but here I see a lot of changes that aren't guiding me.
💬 kinetickinesthetic commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3386978846)
This patch is a highly contentious update. I implore Core to acknowledge that the network is signaling strong resistance to the proposed changes to op_return / datacarriersize.
We — the users and node runners — do not want the filter to be relaxed. The burden of proof is on Core developers to convince us why this change is beneficial. We understand the technical arguments better than you think. I’ve heard Core’s reasoning for the change, and I remain unconvinced. Furthermore, I will pursue othe
...
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3386978846)
This patch is a highly contentious update. I implore Core to acknowledge that the network is signaling strong resistance to the proposed changes to op_return / datacarriersize.
We — the users and node runners — do not want the filter to be relaxed. The burden of proof is on Core developers to convince us why this change is beneficial. We understand the technical arguments better than you think. I’ve heard Core’s reasoning for the change, and I remain unconvinced. Furthermore, I will pursue othe
...
✅ maflcko closed a pull request: "newly Update funcs.mk"
(https://github.com/bitcoin/bitcoin/pull/33590)
(https://github.com/bitcoin/bitcoin/pull/33590)
🤔 janb84 reviewed a pull request: "[30.x] Finalise v30.0"
(https://github.com/bitcoin/bitcoin/pull/33559#pullrequestreview-3320106531)
ACK d615eb6998eeccb9106854dffa95f36b319177e1
- code review ✅
- checked release notes, they are the same except some grammar (line 184), style changes (" => ` etc) and removal of unused chapters / sections. ✅
- cannot generate list of authors, must be doing something wrong with the script.
(https://github.com/bitcoin/bitcoin/pull/33559#pullrequestreview-3320106531)
ACK d615eb6998eeccb9106854dffa95f36b319177e1
- code review ✅
- checked release notes, they are the same except some grammar (line 184), style changes (" => ` etc) and removal of unused chapters / sections. ✅
- cannot generate list of authors, must be doing something wrong with the script.
🤔 l0rinc reviewed a pull request: "build: Bump clang minimum supported version to 17"
(https://github.com/bitcoin/bitcoin/pull/33555#pullrequestreview-3320154679)
Concept ACK
It seems to me there are a few more clang 16 workarounds that I would prefer addressing here.
Do we still need the build workaround in https://github.com/bitcoin/bitcoin/blob/a4ee70e5b69c67c9caea1e97cb5b08f80536c7d3/src/ipc/libmultiprocess/CMakeLists.txt#L56
Can we simplify these tests now
* https://github.com/bitcoin/bitcoin/blob/43cd83b0c7ba436b8ffc83d8a65e935714dffe70/src/test/arith_uint256_tests.cpp#L583-L592
* https://github.com/bitcoin/bitcoin/blob/e50312eab0b54f338d6e08bea
...
(https://github.com/bitcoin/bitcoin/pull/33555#pullrequestreview-3320154679)
Concept ACK
It seems to me there are a few more clang 16 workarounds that I would prefer addressing here.
Do we still need the build workaround in https://github.com/bitcoin/bitcoin/blob/a4ee70e5b69c67c9caea1e97cb5b08f80536c7d3/src/ipc/libmultiprocess/CMakeLists.txt#L56
Can we simplify these tests now
* https://github.com/bitcoin/bitcoin/blob/43cd83b0c7ba436b8ffc83d8a65e935714dffe70/src/test/arith_uint256_tests.cpp#L583-L592
* https://github.com/bitcoin/bitcoin/blob/e50312eab0b54f338d6e08bea
...