💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1992120827)
Good catch. This is wrong, but should be harmless here, albeit highly confusing. Fixed.
Should be trivial to re-ack with `git range-diff bitcoin-core/master A B`
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1992120827)
Good catch. This is wrong, but should be harmless here, albeit highly confusing. Fixed.
Should be trivial to re-ack with `git range-diff bitcoin-core/master A B`
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1992120962)
Not sure what you mean with "retain". In current master there is no static extent, as this is using `Span`.
Also, streams don't make use of static extends at all. See also https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1929225185:
> Presumably some of the streams could benefit from static extents, but that's waaay overkill for here.
So my preference would be to not mix refactoring with questionable performance improvements.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1992120962)
Not sure what you mean with "retain". In current master there is no static extent, as this is using `Span`.
Also, streams don't make use of static extends at all. See also https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1929225185:
> Presumably some of the streams could benefit from static extents, but that's waaay overkill for here.
So my preference would be to not mix refactoring with questionable performance improvements.
💬 maflcko commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1992121110)
> which seems to eliminate the need for
Not sure. Even if this was the only place that makes use of this, for consistency it should be kept anyway. Otherwise, someone will have to touch this file again to add it back once it is used.
(https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1992121110)
> which seems to eliminate the need for
Not sure. Even if this was the only place that makes use of this, for consistency it should be kept anyway. Otherwise, someone will have to touch this file again to add it back once it is used.
💬 maflcko commented on pull request "docs: added a badge to the workflow":
(https://github.com/bitcoin/bitcoin/pull/32032#issuecomment-2718838563)
@fanquake The erroneous code was added in https://github.com/maflcko/DrahtBot/commit/3a7df79dc523e8c602a2da7be526ec14e69e0833 and I missed it during review. Fixed in https://github.com/maflcko/DrahtBot/commit/85c143e08b50ad026290f0b6af5583aff64e1d79. Thanks!
(https://github.com/bitcoin/bitcoin/pull/32032#issuecomment-2718838563)
@fanquake The erroneous code was added in https://github.com/maflcko/DrahtBot/commit/3a7df79dc523e8c602a2da7be526ec14e69e0833 and I missed it during review. Fixed in https://github.com/maflcko/DrahtBot/commit/85c143e08b50ad026290f0b6af5583aff64e1d79. Thanks!
💬 glozow commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2718842557)
Great, fixed.
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2718842557)
Great, fixed.
💬 hebasto commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2718855202)
> > ACK [628e4f5](https://github.com/bitcoin/bitcoin/commit/628e4f59b8784fcde2de176fb98260e71def18ec), I have reviewed the code and it looks OK.
>
> Revoked. Man pages for `bitcoind` and `bitcoin-qt` miss "ZeroMQ notification options"
My apologies for not checking all stuff at once, but `examples/bitcoin.conf` also lacks "ZeroMQ notification options".
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2718855202)
> > ACK [628e4f5](https://github.com/bitcoin/bitcoin/commit/628e4f59b8784fcde2de176fb98260e71def18ec), I have reviewed the code and it looks OK.
>
> Revoked. Man pages for `bitcoind` and `bitcoin-qt` miss "ZeroMQ notification options"
My apologies for not checking all stuff at once, but `examples/bitcoin.conf` also lacks "ZeroMQ notification options".
💬 glozow commented on pull request "[29.x] bump to v29.0rc1":
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2718857240)
Thanks @hebasto!
(https://github.com/bitcoin/bitcoin/pull/32046#issuecomment-2718857240)
Thanks @hebasto!
👍 hebasto approved a pull request: "[29.x] bump to v29.0rc1"
(https://github.com/bitcoin/bitcoin/pull/32046#pullrequestreview-2679617137)
ACK 47e2fa86dc5433852fd9e5050a23de2accfdca8d.
(https://github.com/bitcoin/bitcoin/pull/32046#pullrequestreview-2679617137)
ACK 47e2fa86dc5433852fd9e5050a23de2accfdca8d.
🚀 ryanofsky merged a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283)
(https://github.com/bitcoin/bitcoin/pull/31283)
👍 ryanofsky approved a pull request: "[IBD] flush UXTOs in bigger batches"
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-2679668648)
Code review ACK 868413340f8d6058d74186b65ac3498d6b7f254a
> If that spike exceeds the memory the process can allocate it causes a crash, at a particularly bad time (may require a replay to fix, which may be slower than just reprocessing the blocks).
It is difficult for me to have a sense of how safe this change is but I'd hope we are not currently pushing systems so close to the edge that using an extra 48mb will cause them to start crashing. This does seem like a nice performance improvmen
...
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-2679668648)
Code review ACK 868413340f8d6058d74186b65ac3498d6b7f254a
> If that spike exceeds the memory the process can allocate it causes a crash, at a particularly bad time (may require a replay to fix, which may be slower than just reprocessing the blocks).
It is difficult for me to have a sense of how safe this change is but I'd hope we are not currently pushing systems so close to the edge that using an extra 48mb will cause them to start crashing. This does seem like a nice performance improvmen
...
🚀 hebasto merged a pull request: "wallet: Replace "non-0" with "non-zero" in translatable error message"
(https://github.com/bitcoin/bitcoin/pull/31987)
(https://github.com/bitcoin/bitcoin/pull/31987)
💬 mabu44 commented on pull request "test: Check datadir cleanup after assumeutxo was successful":
(https://github.com/bitcoin/bitcoin/pull/32033#issuecomment-2718935452)
Tested the PR with the following steps:
#### On master:
```bash
python3 feature_assumeutxo.py
...
TestFramework (INFO): Tests successful
```
Modified validation.cpp (line 6237, function MaybeCompleteSnapshotValidation):
```diff
- return SnapshotCompletionResult::SUCCESS;
+ return SnapshotCompletionResult::SKIPPED;
```
This should avoid the clean-up of the snapshot at startup. But the test is not affected:
```bash
python3 feature_assumeutxo.py
...
TestFramework (INFO): Tests succe
...
(https://github.com/bitcoin/bitcoin/pull/32033#issuecomment-2718935452)
Tested the PR with the following steps:
#### On master:
```bash
python3 feature_assumeutxo.py
...
TestFramework (INFO): Tests successful
```
Modified validation.cpp (line 6237, function MaybeCompleteSnapshotValidation):
```diff
- return SnapshotCompletionResult::SUCCESS;
+ return SnapshotCompletionResult::SKIPPED;
```
This should avoid the clean-up of the snapshot at startup. But the test is not affected:
```bash
python3 feature_assumeutxo.py
...
TestFramework (INFO): Tests succe
...
💬 jamesob commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2718981226)
I've added a link to the active [Delving Bitcoin discussion](https://delvingbitcoin.org/t/ctv-csfs-can-we-reach-consensus-on-a-first-step-towards-covenants/1509/23) most pertaining to CTV, and will keep the PR description above updated with any other relevant discussions.
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2718981226)
I've added a link to the active [Delving Bitcoin discussion](https://delvingbitcoin.org/t/ctv-csfs-can-we-reach-consensus-on-a-first-step-towards-covenants/1509/23) most pertaining to CTV, and will keep the PR description above updated with any other relevant discussions.
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992220870)
> hoist up the braces to conform with the dev-notes? Also...inconsistent spacing
> it would be good to use recommended style (can just run `clang-format`)
Good idea, done.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992220870)
> hoist up the braces to conform with the dev-notes? Also...inconsistent spacing
> it would be good to use recommended style (can just run `clang-format`)
Good idea, done.
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992223671)
Oh ok, updated the commit message per s/upper bound/bound/ as you suggested.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992223671)
Oh ok, updated the commit message per s/upper bound/bound/ as you suggested.
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992234119)
See https://stackoverflow.com/questions/3574040/c-can-a-struct-inherit-from-a-class in addition to my response above.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992234119)
See https://stackoverflow.com/questions/3574040/c-can-a-struct-inherit-from-a-class in addition to my response above.
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992240148)
Thank you for clarifying the change. Done.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992240148)
Thank you for clarifying the change. Done.
⚠️ yancyribbens opened an issue: "BnB untested/unused condition in UTXO exclusion optimization"
(https://github.com/bitcoin/bitcoin/issues/32047)
If the following condition is remove, no failure occurs:
`utxo.fee != utxo_pool.at(utxo_pool_index - 1).fee)`
https://github.com/bitcoin/bitcoin/blob/eb9730ab65887279309af338ad0201924f945ac5/src/wallet/coinselection.cpp#L177
Furthermore, I believe that this condition isn't actually needed and can be removed, which would slightly boost performance. The section is checking if a UTXO can be omitted from evaluation if it has the same effective_value as the previous UTXO and the previous was alrea
...
(https://github.com/bitcoin/bitcoin/issues/32047)
If the following condition is remove, no failure occurs:
`utxo.fee != utxo_pool.at(utxo_pool_index - 1).fee)`
https://github.com/bitcoin/bitcoin/blob/eb9730ab65887279309af338ad0201924f945ac5/src/wallet/coinselection.cpp#L177
Furthermore, I believe that this condition isn't actually needed and can be removed, which would slightly boost performance. The section is checking if a UTXO can be omitted from evaluation if it has the same effective_value as the previous UTXO and the previous was alrea
...
💬 jonatack commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#issuecomment-2719061577)
Thank you for reviewing, @pablomartin4btc. I prefer to keep this pull scoped only to this file and not increase the scope, as these changes are local to it and it is a file I know reasonably well (the RequestHandler representation hasn't changed over that time period).
(https://github.com/bitcoin/bitcoin/pull/31887#issuecomment-2719061577)
Thank you for reviewing, @pablomartin4btc. I prefer to keep this pull scoped only to this file and not increase the scope, as these changes are local to it and it is a file I know reasonably well (the RequestHandler representation hasn't changed over that time period).
💬 hodlinator commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992250931)
My objection does not come from a knowledge gap in what C++ can and cannot do. It is about sticking to certain conventions.
As I was checking to see if the Core Guidelines had influenced me in the matter I only found weak counter evidence:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c122-use-abstract-classes-as-interfaces-when-complete-separation-of-interface-and-implementation-is-needed
In the example code `class D1` inherits from `struct Device`, full of virtual methods.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1992250931)
My objection does not come from a knowledge gap in what C++ can and cannot do. It is about sticking to certain conventions.
As I was checking to see if the Core Guidelines had influenced me in the matter I only found weak counter evidence:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c122-use-abstract-classes-as-interfaces-when-complete-separation-of-interface-and-implementation-is-needed
In the example code `class D1` inherits from `struct Device`, full of virtual methods.