💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2611171333)
094e845f7309c8cf4e172c8fe07e200e054ca2db
TIL, nice
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2611171333)
094e845f7309c8cf4e172c8fe07e200e054ca2db
TIL, nice
💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2611697771)
c2eaf22503f7af057599f02ecb5f24a267ea33e2
suggestion:
```
- /** The set of all transactions in the cluster. */
- SetType m_transactions;
- /** Information about each transaction (and chunks). Indexed by TxIdx. */
+ /** The set of all TxIdx's of transactions in the cluster indexing into m_tx_data. */
+ SetType m_transaction_idxs;
+ /** Information about each transaction (and chunks). Keeps the "holes" from DepGraph
+ during construction. Indexed by TxIdx. */
...
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2611697771)
c2eaf22503f7af057599f02ecb5f24a267ea33e2
suggestion:
```
- /** The set of all transactions in the cluster. */
- SetType m_transactions;
- /** Information about each transaction (and chunks). Indexed by TxIdx. */
+ /** The set of all TxIdx's of transactions in the cluster indexing into m_tx_data. */
+ SetType m_transaction_idxs;
+ /** Information about each transaction (and chunks). Keeps the "holes" from DepGraph
+ during construction. Indexed by TxIdx. */
...
💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2614950800)
a0cb73b4257f771353f6732e7afdaf2d03b51efe
now that UpdateChunk has two TxIdx args let's annotate themhere and other three spots
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2614950800)
a0cb73b4257f771353f6732e7afdaf2d03b51efe
now that UpdateChunk has two TxIdx args let's annotate themhere and other three spots
💬 instagibbs commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615158883)
a0cb73b4257f771353f6732e7afdaf2d03b51efe
IIUC these two invocations are optimizations we can use since we know where things are potentially mergable next instead of `MakeTopological`?
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615158883)
a0cb73b4257f771353f6732e7afdaf2d03b51efe
IIUC these two invocations are optimizations we can use since we know where things are potentially mergable next instead of `MakeTopological`?
💬 mzumsande commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2615582635)
done
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2615582635)
done
💬 mzumsande commented on pull request "validation: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2615583174)
After [this discussion](https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586270472) in which we discussed the separate topic that the current `InvalidChainFound()` call in `ActivateBestChainStep()` often does repeated work and logging, I decided to drop the commit, and will work on a PR to reorder things instead. Current rough plan is to move most things from `InvalidChainFound` to `InvalidBlockFound` and rename the former to `UpdateBestInvalid()`.
For this PR, I have instead added
...
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2615583174)
After [this discussion](https://github.com/bitcoin/bitcoin/pull/31533#discussion_r2586270472) in which we discussed the separate topic that the current `InvalidChainFound()` call in `ActivateBestChainStep()` often does repeated work and logging, I decided to drop the commit, and will work on a PR to reorder things instead. Current rough plan is to move most things from `InvalidChainFound` to `InvalidBlockFound` and rename the former to `UpdateBestInvalid()`.
For this PR, I have instead added
...
💬 mzumsande commented on pull request "doc: invalid block handling followups":
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2615586804)
I added a sentence with the basics (didn't include the describe the different orderings).
(https://github.com/bitcoin/bitcoin/pull/32843#discussion_r2615586804)
I added a sentence with the basics (didn't include the describe the different orderings).
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615619523)
> can this result in an infinite loop or disconnect/reconnects?
@l0rinc hmm possibly. The next connection will get a `nullopt` `opt_tx` in `PushPrivateBroadcastTx` and disconnect, and then here when finalizing it will return `false` for `DidNodeConfirmReception` again and keep reopening. I think here we should just check if there are any txs in the private broadcast queue, not that the node's specific tx is still in the private broadcast queue.
```suggestion
if (node.IsPrivateBroadcast
...
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615619523)
> can this result in an infinite loop or disconnect/reconnects?
@l0rinc hmm possibly. The next connection will get a `nullopt` `opt_tx` in `PushPrivateBroadcastTx` and disconnect, and then here when finalizing it will return `false` for `DidNodeConfirmReception` again and keep reopening. I think here we should just check if there are any txs in the private broadcast queue, not that the node's specific tx is still in the private broadcast queue.
```suggestion
if (node.IsPrivateBroadcast
...
💬 l0rinc commented on pull request "Make `transaction_indentifier` hex string constructor evaluated at comptime":
(https://github.com/bitcoin/bitcoin/pull/34063#issuecomment-3648279574)
It seems you've hit https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117501
It seems we can fix it with an explicit `constexpr` for the declarations, see: https://godbolt.org/z/xb5TMaPs6
I have pushed an alternative to https://github.com/l0rinc/bitcoin/pull/63, please cherry-pick it if it passes CI.
Note that I've also changed some constructors to use brace init and extended the commit message with the issue and the godbolt reproducer.
(https://github.com/bitcoin/bitcoin/pull/34063#issuecomment-3648279574)
It seems you've hit https://gcc.gnu.org/bugzilla/show_bug.cgi?id=117501
It seems we can fix it with an explicit `constexpr` for the declarations, see: https://godbolt.org/z/xb5TMaPs6
I have pushed an alternative to https://github.com/l0rinc/bitcoin/pull/63, please cherry-pick it if it passes CI.
Note that I've also changed some constructors to use brace init and extended the commit message with the issue and the godbolt reproducer.
💬 l0rinc commented on pull request "Implementation of SwiftSync":
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2615700914)
> added you as a commit co-author
You need to add a [coauthor](https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors#creating-co-authored-commits-on-the-command-line) with a fixed format, e.g. https://github.com/bitcoin/bitcoin/commit/8e4c66d0a7a0911c10dced0d6dd60ca7bd9545af
(https://github.com/bitcoin/bitcoin/pull/34004#discussion_r2615700914)
> added you as a commit co-author
You need to add a [coauthor](https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors#creating-co-authored-commits-on-the-command-line) with a fixed format, e.g. https://github.com/bitcoin/bitcoin/commit/8e4c66d0a7a0911c10dced0d6dd60ca7bd9545af
💬 ajtowns commented on pull request "net: Waste less time in socket handling":
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2615702010)
Duration works as an atomic fine -- we have `atomic<seconds>` and `atomic<microseconds>` in various places in net, net_processing and util/time. `atomic<time_point>` doesn't work because even if the duration is `noexcept` (necessary for atomic), that doesn't propagate to the time_point's constructor also being noexcept which is a requirement for wrapping in atomic.
(https://github.com/bitcoin/bitcoin/pull/34025#discussion_r2615702010)
Duration works as an atomic fine -- we have `atomic<seconds>` and `atomic<microseconds>` in various places in net, net_processing and util/time. `atomic<time_point>` doesn't work because even if the duration is `noexcept` (necessary for atomic), that doesn't propagate to the time_point's constructor also being noexcept which is a requirement for wrapping in atomic.
💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615782689)
> I think here we should just check if there are any txs in the private broadcast queue, not that the node's specific tx is still in the private broadcast queue.
That makes sense. Having additional attempts that would be spent on other transactions in the worst case is no big deal in my opinion, but obviously the infinite loop possibility should be fixed.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2615782689)
> I think here we should just check if there are any txs in the private broadcast queue, not that the node's specific tx is still in the private broadcast queue.
That makes sense. Having additional attempts that would be spent on other transactions in the worst case is no big deal in my opinion, but obviously the infinite loop possibility should be fixed.
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615788225)
I have reverted this change. My benchmarks show that the time per cost is quite similar to before, which I'm now mentioning in the commit message. This will be overhauled in #34023.
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615788225)
I have reverted this change. My benchmarks show that the time per cost is quite similar to before, which I'm now mentioning in the commit message. This will be overhauled in #34023.
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615788367)
Done.
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615788367)
Done.
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615788964)
I have just increased it to 1T iterations, which ought to be enough for anyone.
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615788964)
I have just increased it to 1T iterations, which ought to be enough for anyone.
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615789740)
Done, except renaming to `m_transaction_idxs`, because I'm too lazy to update all the affected commits right now. Will leave this open.
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615789740)
Done, except renaming to `m_transaction_idxs`, because I'm too lazy to update all the affected commits right now. Will leave this open.
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615790625)
It's to avoid reallocating; this formula gives the maximum number of dependencies a cluster with a given number of transactions can have. I have added a comment. Does this suffice?
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615790625)
It's to avoid reallocating; this formula gives the maximum number of dependencies a cluster with a given number of transactions can have. I have added a comment. Does this suffice?
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615790818)
Indeed! Added as comment.
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615790818)
Indeed! Added as comment.
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615791108)
I was considering that already, done.
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615791108)
I was considering that already, done.
💬 sipa commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615793339)
The point here is to prevent against potential bugs added in the future, which result in optimality just never being reached anymore. Absent bounds on expected iteration count, this would render a large portion of this test powerless (because anything in `is_optimal`, which includes all quality checks) won't be reached anymore.
Added a comment. Does this explain it?
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2615793339)
The point here is to prevent against potential bugs added in the future, which result in optimality just never being reached anymore. Absent bounds on expected iteration count, this would render a large portion of this test powerless (because anything in `is_optimal`, which includes all quality checks) won't be reached anymore.
Added a comment. Does this explain it?
💬 ajtowns commented on pull request "Make `transaction_indentifier` hex string constructor evaluated at comptime":
(https://github.com/bitcoin/bitcoin/pull/34063#discussion_r2615793433)
Would it be better to provide an `operator""_txid` and `_wtxid` along the lines of hexliterals in util/strencodings.h? They could be made available only to test code that way.
Adding `contstexpr` to the constructors in primitives/transaction_identifier.h and writing:
```c++
consteval auto operator""_txid(const char* str, size_t n) { return Txid::FromUint256(uint256{std::string_view{str, n}}); }
consteval auto operator""_wtxid(const char* str, size_t n) { return Wtxid::FromUint256(uint256
...
(https://github.com/bitcoin/bitcoin/pull/34063#discussion_r2615793433)
Would it be better to provide an `operator""_txid` and `_wtxid` along the lines of hexliterals in util/strencodings.h? They could be made available only to test code that way.
Adding `contstexpr` to the constructors in primitives/transaction_identifier.h and writing:
```c++
consteval auto operator""_txid(const char* str, size_t n) { return Txid::FromUint256(uint256{std::string_view{str, n}}); }
consteval auto operator""_wtxid(const char* str, size_t n) { return Wtxid::FromUint256(uint256
...