💬 purpleKarrot commented on pull request "cmake: Specify Windows plugin path in `test_bitcoin-qt` property":
(https://github.com/bitcoin/bitcoin/pull/33865#discussion_r2529975644)
> it returns the path to the “Release” plugin when building with `--config Debug`.
Interesting. I put that in my backlog. Low priority, because your approach is working. I just want to know why the `LOCATION` property is not set correctly.
(https://github.com/bitcoin/bitcoin/pull/33865#discussion_r2529975644)
> it returns the path to the “Release” plugin when building with `--config Debug`.
Interesting. I put that in my backlog. Low priority, because your approach is working. I just want to know why the `LOCATION` property is not set correctly.
💬 optout21 commented on pull request "refactor: remove incorrect lifetimebounds":
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2529976285)
An off-topic nit: the `current_tip` value technically can be nullptr, and this is not checked here and not handled inside `GetFirstBlock`. However, this can happen only on an empty chain, and the behavior is not touched by this PR.
(https://github.com/bitcoin/bitcoin/pull/33870#discussion_r2529976285)
An off-topic nit: the `current_tip` value technically can be nullptr, and this is not checked here and not handled inside `GetFirstBlock`. However, this can happen only on an empty chain, and the behavior is not touched by this PR.
💬 stringintech commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3536633344)
This occurred to me while [refactoring](https://github.com/stringintech/go-bitcoinkernel/pull/9) the context and chainstate manager options in the Go wrapper, so I opened this PR to get feedback.
cc @TheCharlatan @stickies-v
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3536633344)
This occurred to me while [refactoring](https://github.com/stringintech/go-bitcoinkernel/pull/9) the context and chainstate manager options in the Go wrapper, so I opened this PR to get feedback.
cc @TheCharlatan @stickies-v
🤔 optout21 reviewed a pull request: "refactor: remove incorrect lifetimebounds"
(https://github.com/bitcoin/bitcoin/pull/33870#pullrequestreview-3468175235)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
Review, local build, unit tests.
The change has two small code-health improvements; first concerns compiler lifetime hints and can be validated by code review. The other change -- change of return value from pointer to reference reduces chance of null pointer mishandling. The affected method is used in a few places, all are checked, so the impact is minimal.
(https://github.com/bitcoin/bitcoin/pull/33870#pullrequestreview-3468175235)
ACK 99d012ec80a4415e1a37218fb4933550276b9a0a
Review, local build, unit tests.
The change has two small code-health improvements; first concerns compiler lifetime hints and can be validated by code review. The other change -- change of return value from pointer to reference reduces chance of null pointer mishandling. The affected method is used in a few places, all are checked, so the impact is minimal.
🚀 hebasto merged a pull request: "cmake: Specify Windows plugin path in `test_bitcoin-qt` property"
(https://github.com/bitcoin/bitcoin/pull/33865)
(https://github.com/bitcoin/bitcoin/pull/33865)
💬 optout21 commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2529998066)
Can a check on `dirty_count` be added after the `BatchWrite` below?
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2529998066)
Can a check on `dirty_count` be added after the `BatchWrite` below?
💬 optout21 commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2530000189)
Misleading comment, no calculation takes place here, it's just an accessor.
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2530000189)
Misleading comment, no calculation takes place here, it's just an accessor.
💬 optout21 commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2530010319)
I suggest moving this line one line down, such that `m_dirty_count` and `cachedCoinsUsage` updates are on adjacent lines, as they are logically related.
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2530010319)
I suggest moving this line one line down, such that `m_dirty_count` and `cachedCoinsUsage` updates are on adjacent lines, as they are logically related.
💬 optout21 commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#issuecomment-3536685676)
A general observation: keeping a count like the dirty count is very efficient, but prone to going out of sync, e.g with future incorrect code changes. For eg., a missing decrement can result in the value creeping higher, a missing increment can result the value going lower, event to underflow -- `size_t` is unsigned. One mitigation is to create (inlinable) inc/dec methods, with sanity checks (in debug mode); pre-decrease the value cannot be 0, pre-increase it cannot be equal to the total entries
...
(https://github.com/bitcoin/bitcoin/pull/33512#issuecomment-3536685676)
A general observation: keeping a count like the dirty count is very efficient, but prone to going out of sync, e.g with future incorrect code changes. For eg., a missing decrement can result in the value creeping higher, a missing increment can result the value going lower, event to underflow -- `size_t` is unsigned. One mitigation is to create (inlinable) inc/dec methods, with sanity checks (in debug mode); pre-decrease the value cannot be 0, pre-increase it cannot be equal to the total entries
...
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >20% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3536696051)
What about capping it to min of 8 or `-par` value if set to something other than 0. If `-par` is 0, set to 8? I think extra threads will help here even on a single core system.
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3536696051)
What about capping it to min of 8 or `-par` value if set to something other than 0. If `-par` is 0, set to 8? I think extra threads will help here even on a single core system.
💬 fjahr commented on pull request "test, refactor: Embedded ASmap selected preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3536862669)
> although I would also consider peeling off bits->bytes refactor and span-usage to its own PR
It's a good idea, I will take it especially because I have been working on improved documentation for the asmap internals and this would go along nicely with this part of the original PR, I think. Since I started this restructuring yesterday already I will push this soon without your latest review here addressed: https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3448784895 (thank you f
...
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3536862669)
> although I would also consider peeling off bits->bytes refactor and span-usage to its own PR
It's a good idea, I will take it especially because I have been working on improved documentation for the asmap internals and this would go along nicely with this part of the original PR, I think. Since I started this restructuring yesterday already I will push this soon without your latest review here addressed: https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3448784895 (thank you f
...
📝 fjahr opened a pull request: "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation"
(https://github.com/bitcoin/bitcoin/pull/33878)
Depends on #33026, the prior part in this series. Please review this one first.
This is a second slice carved out of #28792. It contains the following changes that are crucial for the embedding of asmap data which is added the following PR in the series (probably this will remain in #28792).
The changes are:
- Modernizes and simplifies the asmap code by operating on `std::byte` instead of bits
- Unifies asmap version calculation and naming (previously it was called version and checksum i
...
(https://github.com/bitcoin/bitcoin/pull/33878)
Depends on #33026, the prior part in this series. Please review this one first.
This is a second slice carved out of #28792. It contains the following changes that are crucial for the embedding of asmap data which is added the following PR in the series (probably this will remain in #28792).
The changes are:
- Modernizes and simplifies the asmap code by operating on `std::byte` instead of bits
- Unifies asmap version calculation and naming (previously it was called version and checksum i
...
⚠️ fjahr opened an issue: "Embedded ASMap data Tracking Issue (Part 2)"
(https://github.com/bitcoin/bitcoin/issues/33879)
The predecessor tracking issue for this topic can be found here: #28794 This was closed since there seemed to be just one main PR left but I am reviving a topic issue for asmap because recently a few PRs have spun out of the main PR and related discussions. Aside from the somewhat tangential changes the main embedding PR is split into a series of three as of now.
- Embedding implementation series
- [ ] #33026
- [ ] #33857
- [ ] #28792
- [ ] https://github.com/bitcoin/bitcoin/issues/3338
...
(https://github.com/bitcoin/bitcoin/issues/33879)
The predecessor tracking issue for this topic can be found here: #28794 This was closed since there seemed to be just one main PR left but I am reviving a topic issue for asmap because recently a few PRs have spun out of the main PR and related discussions. Aside from the somewhat tangential changes the main embedding PR is split into a series of three as of now.
- Embedding implementation series
- [ ] #33026
- [ ] #33857
- [ ] #28792
- [ ] https://github.com/bitcoin/bitcoin/issues/3338
...
🤔 furszy reviewed a pull request: "qa: Account for unset errno in ConnectionResetError"
(https://github.com/bitcoin/bitcoin/pull/33875#pullrequestreview-3468346292)
Tested ACK 76e0e6087d0310ec31f43d751de60adf0c0a2faa
Thanks for improving this. It makes understanding internal issues simpler.
It’d be nice if the upcoming http server implementation caught these unhandled exceptions, writes to `std::cerr` and returns a proper error instead of exposing the internal one via the http response. Tagging @pinheadmz.
(https://github.com/bitcoin/bitcoin/pull/33875#pullrequestreview-3468346292)
Tested ACK 76e0e6087d0310ec31f43d751de60adf0c0a2faa
Thanks for improving this. It makes understanding internal issues simpler.
It’d be nice if the upcoming http server implementation caught these unhandled exceptions, writes to `std::cerr` and returns a proper error instead of exposing the internal one via the http response. Tagging @pinheadmz.
💬 furszy commented on pull request "qa: Account for unset errno in ConnectionResetError":
(https://github.com/bitcoin/bitcoin/pull/33875#discussion_r2530131801)
nit: might want to update the line below to state this could also happen due to an uncaught exception at the server side. Talking about this:
```python
errno.ECONNRESET, # This might happen when the RPC server is in warmup
```
(https://github.com/bitcoin/bitcoin/pull/33875#discussion_r2530131801)
nit: might want to update the line below to state this could also happen due to an uncaught exception at the server side. Talking about this:
```python
errno.ECONNRESET, # This might happen when the RPC server is in warmup
```
💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3536888674)
I am taking this PR back into draft status temporarily because I have been slicing it into 3 parts thanks to the suggestions of @hodlinator . The first part is #33026 which features some minor prep work and is ready for review right now. The second part is #33878 which can be reviewed now but has not addressed the [latest feedback from today](https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3448784895). There is one new commit in there which I hope is interesting: it adds further
...
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-3536888674)
I am taking this PR back into draft status temporarily because I have been slicing it into 3 parts thanks to the suggestions of @hodlinator . The first part is #33026 which features some minor prep work and is ready for review right now. The second part is #33878 which can be reviewed now but has not addressed the [latest feedback from today](https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3448784895). There is one new commit in there which I hope is interesting: it adds further
...
📝 fjahr converted_to_draft a pull request: "Embedded ASMap [3/3]: Build binary dump header file"
(https://github.com/bitcoin/bitcoin/pull/28792)
This is a step towards making asmap usage more accessible. Currently an asmap file needs to be acquired by there user from some location or the user needs to generate one themselves. Then they need to move the file to the right place in datadir or pass the path to the file as `-asmap=PATH` in order to use the asmap feature.
The change here allows for builds to embed asmap data into the bitcoind binary which makes it possible to use the feature without handling of the asmap file by the user. I
...
(https://github.com/bitcoin/bitcoin/pull/28792)
This is a step towards making asmap usage more accessible. Currently an asmap file needs to be acquired by there user from some location or the user needs to generate one themselves. Then they need to move the file to the right place in datadir or pass the path to the file as `-asmap=PATH` in order to use the asmap feature.
The change here allows for builds to embed asmap data into the bitcoind binary which makes it possible to use the feature without handling of the asmap file by the user. I
...
💬 fjahr commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3536890738)
@sipa would love to get your eyes on the [documentation commit](https://github.com/bitcoin/bitcoin/pull/33878/commits/93205dd90f525a42f9e41cadb289c4075c5fca09) before anyone else gets confused by the mistakes I might have made :)
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3536890738)
@sipa would love to get your eyes on the [documentation commit](https://github.com/bitcoin/bitcoin/pull/33878/commits/93205dd90f525a42f9e41cadb289c4075c5fca09) before anyone else gets confused by the mistakes I might have made :)
💬 fjahr commented on pull request "Embedded ASMap [3/3]: Build binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2530154158)
I can't remember encountering such a tool but primarily I was going by our style guide, which I thought had mentioned that we don't do new lines before EOF but I guess it doesn't (anymore?) since I couldn't find it right now. At least we have `InsertNewlineAtEOF: false` in our `.clang-format`.
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2530154158)
I can't remember encountering such a tool but primarily I was going by our style guide, which I thought had mentioned that we don't do new lines before EOF but I guess it doesn't (anymore?) since I couldn't find it right now. At least we have `InsertNewlineAtEOF: false` in our `.clang-format`.
💬 sipa commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2530246100)
All the documentation changes in this commit look correct to me.
One thing that may be worth mentioning early on is that it's a bit-packed format, i.e., the entire compressed mapping is treated as a sequence of bits, packed together at 8 per byte, which are concatenated encodings of instructions.
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2530246100)
All the documentation changes in this commit look correct to me.
One thing that may be worth mentioning early on is that it's a bit-packed format, i.e., the entire compressed mapping is treated as a sequence of bits, packed together at 8 per byte, which are concatenated encodings of instructions.