👍 brunoerg approved a pull request: "test: add functional test for `TestShell` (matching doc example)"
(https://github.com/bitcoin/bitcoin/pull/33546#pullrequestreview-3319800382)
ACK 57f7c68821d96cc096db624cb06b7a252d038300
(https://github.com/bitcoin/bitcoin/pull/33546#pullrequestreview-3319800382)
ACK 57f7c68821d96cc096db624cb06b7a252d038300
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417370157)
Yeah, extracting is seen as moving to the "none" quality level.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417370157)
Yeah, extracting is seen as moving to the "none" quality level.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417374122)
This matches the code that's already in GenericClusterImpl.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417374122)
This matches the code that's already in GenericClusterImpl.
💬 ryanofsky commented on issue "RFC: Cancelling waitNext calls in the IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3386691100)
re: https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3386438916
> I guess in theory they could be unified into a single loop, however `interruptWait` would have to be called whenever **some** incoming Sv2 message arrived, not only if that was a `CoinbaseOutputConstraints`.
This is surprising to me, but it sounds like this justifies using two loops to avoid pointless `interruptWait` and `waitNext` churn.
In python pseudocode above, waitNext promise is stored in the `next_block` var
...
(https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3386691100)
re: https://github.com/bitcoin/bitcoin/issues/33575#issuecomment-3386438916
> I guess in theory they could be unified into a single loop, however `interruptWait` would have to be called whenever **some** incoming Sv2 message arrived, not only if that was a `CoinbaseOutputConstraints`.
This is surprising to me, but it sounds like this justifies using two loops to avoid pointless `interruptWait` and `waitNext` churn.
In python pseudocode above, waitNext promise is stored in the `next_block` var
...
💬 instagibbs commented on pull request "[30.x] Finalise v30.0":
(https://github.com/bitcoin/bitcoin/pull/33559#issuecomment-3386708420)
ACK d615eb6998eeccb9106854dffa95f36b319177e1
reviewed the release notes
(https://github.com/bitcoin/bitcoin/pull/33559#issuecomment-3386708420)
ACK d615eb6998eeccb9106854dffa95f36b319177e1
reviewed the release notes
💬 glozow commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417392577)
Fwiw I don't agree with this either as we are not scheduling anything, and ForceRelay is the name of the permission.
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417392577)
Fwiw I don't agree with this either as we are not scheduling anything, and ForceRelay is the name of the permission.
💬 theStack commented on pull request "Rollback for dumptxoutset without invalidating blocks":
(https://github.com/bitcoin/bitcoin/pull/33477#issuecomment-3386727879)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33477#issuecomment-3386727879)
Concept ACK
📝 bavani-2024-aia opened a pull request: "config.ini headers missing causes test framework failure"
(https://github.com/bitcoin/bitcoin/pull/33588)
The functional tests in the repository fail because the config.ini file is missing proper section headers, causing the test framework to throw a MissingSectionHeaderError.
This occurs when running tests like mempool_ephemeral_dust.py, producing the following error:
configparser.MissingSectionHeaderError: File contains no section headers.
file: '.../test/config.ini', line: 1
'[environment]\n'
The root cause is either a missing [environment] header or an invisible BOM (Byte Order M
...
(https://github.com/bitcoin/bitcoin/pull/33588)
The functional tests in the repository fail because the config.ini file is missing proper section headers, causing the test framework to throw a MissingSectionHeaderError.
This occurs when running tests like mempool_ephemeral_dust.py, producing the following error:
configparser.MissingSectionHeaderError: File contains no section headers.
file: '.../test/config.ini', line: 1
'[environment]\n'
The root cause is either a missing [environment] header or an invisible BOM (Byte Order M
...
📝 bavani-2024-aia opened a pull request: "config.ini headers missing causes test framework failure"
(https://github.com/bitcoin/bitcoin/pull/33589)
The functional tests in the repository fail because the config.ini file is missing proper section headers, causing the test framework to throw a MissingSectionHeaderError.
This occurs when running tests like mempool_ephemeral_dust.py, producing the following error:
configparser.MissingSectionHeaderError: File contains no section headers.
file: '.../test/config.ini', line: 1
'[environment]\n'
The root cause is either a missing [environment] header or an invisible BOM (Byte Order M
...
(https://github.com/bitcoin/bitcoin/pull/33589)
The functional tests in the repository fail because the config.ini file is missing proper section headers, causing the test framework to throw a MissingSectionHeaderError.
This occurs when running tests like mempool_ephemeral_dust.py, producing the following error:
configparser.MissingSectionHeaderError: File contains no section headers.
file: '.../test/config.ini', line: 1
'[environment]\n'
The root cause is either a missing [environment] header or an invisible BOM (Byte Order M
...
🤔 andrewtoth reviewed a pull request: "node: change a tx-relay on/off flag to enum"
(https://github.com/bitcoin/bitcoin/pull/33567#pullrequestreview-3319681241)
Not sure we should add comments (yet) that suggest adding to the mempool is optional. With this change it is still always added unless it already exists. In the change for private broadcast there are a few comments that can be updated.
(https://github.com/bitcoin/bitcoin/pull/33567#pullrequestreview-3319681241)
Not sure we should add comments (yet) that suggest adding to the mempool is optional. With this change it is still always added unless it already exists. In the change for private broadcast there are a few comments that can be updated.
💬 andrewtoth commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2417297799)
The transaction is passed by const ref, so it's not really "consuming" it. Perhaps "Process a local transaction"?
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2417297799)
The transaction is passed by const ref, so it's not really "consuming" it. Perhaps "Process a local transaction"?
💬 andrewtoth commented on pull request "node: change a tx-relay on/off flag to enum":
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2417280933)
This parameter does not allow the caller to decide whether to add the transaction to the mempool (yet).
(https://github.com/bitcoin/bitcoin/pull/33567#discussion_r2417280933)
This parameter does not allow the caller to decide whether to add the transaction to the mempool (yet).
💬 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).