💬 sipa commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766882491)
Someone shouldn't need to look at the implementation to understand what a function does.
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766882491)
Someone shouldn't need to look at the implementation to understand what a function does.
💬 l0rinc commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766898995)
> The advantage of uint64_t (and presumably the reason you're suggesting this) is because it signals that the number cannot be negative.
Yes, to make it stricter and closer aligned to the problem domain.
> signed types have the advantage that they actually model integers
But here we're modelling a size where potential negative values are confusing (especially since we're storing it as unsigned int going forward).
The negative values were already handled, it seems to me that they lost t
...
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766898995)
> The advantage of uint64_t (and presumably the reason you're suggesting this) is because it signals that the number cannot be negative.
Yes, to make it stricter and closer aligned to the problem domain.
> signed types have the advantage that they actually model integers
But here we're modelling a size where potential negative values are confusing (especially since we're storing it as unsigned int going forward).
The negative values were already handled, it seems to me that they lost t
...
💬 sipa commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766913104)
@l0rinc All the same arguments apply to the size of spans, which cannot be negative, and yet people strongly (and unsuccessfully, but only barely) argued that it should have a signed size type. I don't believe the concern is about specific worries around overflow and its behavior; it is about the fact that sizes shouldn't be thought of as modular arithmetic, and that C++ has no type to model natural numbers - the best approximation is signed types which model integers.
And again, I also see t
...
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766913104)
@l0rinc All the same arguments apply to the size of spans, which cannot be negative, and yet people strongly (and unsuccessfully, but only barely) argued that it should have a signed size type. I don't believe the concern is about specific worries around overflow and its behavior; it is about the fact that sizes shouldn't be thought of as modular arithmetic, and that C++ has no type to model natural numbers - the best approximation is signed types which model integers.
And again, I also see t
...
💬 l0rinc commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766913861)
I'm not exactly sure in which cases this comment would help us avoid having to check the sources - but I don't have strong opinions about it, please resolve this comment.
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766913861)
I'm not exactly sure in which cases this comment would help us avoid having to check the sources - but I don't have strong opinions about it, please resolve this comment.
💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2361124220)
Added a CMake build flag `-DWITH_SV2` that's `OFF` by default.
Also rebased and addressed comments.
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2361124220)
Added a CMake build flag `-DWITH_SV2` that's `OFF` by default.
Also rebased and addressed comments.
💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1766927347)
Taken
(https://github.com/bitcoin/bitcoin/pull/29346#discussion_r1766927347)
Taken
👍 theStack approved a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315710653)
Code-review ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315710653)
Code-review ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369
💬 l0rinc commented on pull request "Follow-up after AutoFile position caching: remove unused code":
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766933675)
> Mixing signed and unsigned numbers is a common source of confusion and bugs. Most coding guidelines recommend against mixing them.
But we *are* mixing them, that's my main concern
```C++
pos.nPos = (unsigned int)fileOutPos
```
I'd be fine with using a signed position in both cases, but if one is signed and the other is not, most of us will assume that we have to check the signed value for negatives (as the existence of this PR demonstrates).
But changing the usages is outside the sco
...
(https://github.com/bitcoin/bitcoin/pull/30927#discussion_r1766933675)
> Mixing signed and unsigned numbers is a common source of confusion and bugs. Most coding guidelines recommend against mixing them.
But we *are* mixing them, that's my main concern
```C++
pos.nPos = (unsigned int)fileOutPos
```
I'd be fine with using a signed position in both cases, but if one is signed and the other is not, most of us will assume that we have to check the signed value for negatives (as the existence of this PR demonstrates).
But changing the usages is outside the sco
...
👍 l0rinc approved a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315718035)
ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369
My only remaining concern is lack of related test coverage, but it's not a blocker
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315718035)
ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369
My only remaining concern is lack of related test coverage, but it's not a blocker
💬 sipa commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1766950748)
> Many wallets will ultimately want to show which address is being spent from, e.g.
The fact that people want to do dumb things is not a reason to support it. I blame blockchain explorers for instituting, and perpetuating, the misunderstanding that Bitcoin wallet balances can be assessed by observing their receive addresses' balances.
Bitcoin Core has never followed the notion that addresses have their own individual balance or can be spent "from" (it's not wrong or inconsistent to see thi
...
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1766950748)
> Many wallets will ultimately want to show which address is being spent from, e.g.
The fact that people want to do dumb things is not a reason to support it. I blame blockchain explorers for instituting, and perpetuating, the misunderstanding that Bitcoin wallet balances can be assessed by observing their receive addresses' balances.
Bitcoin Core has never followed the notion that addresses have their own individual balance or can be spent "from" (it's not wrong or inconsistent to see thi
...
💬 maflcko commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361181143)
> fix should be simple -- make sure that mutex is held until the ZMQ alert is finished. A new RPC call that comes in should block until the Chainstate update (including announcement over ZMQ) is complete.
That'd block Bitcoin Core from making any progress in the meantime. When it comes to connecting new blocks to the tip, announcing/relaying, and starting to mine on them, no additional delay is acceptable, because for miners any delay means that they'd work on a stale block.
> Why cann
...
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361181143)
> fix should be simple -- make sure that mutex is held until the ZMQ alert is finished. A new RPC call that comes in should block until the Chainstate update (including announcement over ZMQ) is complete.
That'd block Bitcoin Core from making any progress in the meantime. When it comes to connecting new blocks to the tip, announcing/relaying, and starting to mine on them, no additional delay is acceptable, because for miners any delay means that they'd work on a stale block.
> Why cann
...
📝 stickies-v opened a pull request: "util: refactor: add and use run-time safe tinyformat::try_format"
(https://github.com/bitcoin/bitcoin/pull/30928)
In certain situations, such as logging, string formatting errors should not cause application failures. We already use `util::ConstevalFormatString` in those cases, to try and catch some errors at compile-time.
Improve this further by suppressing run-time errors by returning the error message instead of throwing a `tinyformat::format_error`. This is done by taking the existing error handling in `LogPrintFormatInternal()`, and move it to `tfm::try_format()`.
This PR should introduce no beha
...
(https://github.com/bitcoin/bitcoin/pull/30928)
In certain situations, such as logging, string formatting errors should not cause application failures. We already use `util::ConstevalFormatString` in those cases, to try and catch some errors at compile-time.
Improve this further by suppressing run-time errors by returning the error message instead of throwing a `tinyformat::format_error`. This is done by taking the existing error handling in `LogPrintFormatInternal()`, and move it to `tfm::try_format()`.
This PR should introduce no beha
...
💬 sipa commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361213571)
My advice would be to never ever look at the contents of ZMQ notifications, but just see them as notifications that new information may be available, so you should go check and see for yourself. Given the fact that ZMQ notifications can be delayed (see this example) or even dropped entirely (if the queue grows too large, ZMQ has no guarantees AFAIK), you:
* need to check for updated information anyway whenever a notification comes in
* need a mechanism to check occasionally even without notifi
...
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361213571)
My advice would be to never ever look at the contents of ZMQ notifications, but just see them as notifications that new information may be available, so you should go check and see for yourself. Given the fact that ZMQ notifications can be delayed (see this example) or even dropped entirely (if the queue grows too large, ZMQ has no guarantees AFAIK), you:
* need to check for updated information anyway whenever a notification comes in
* need a mechanism to check occasionally even without notifi
...
💬 mcelrath commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361214383)
Working on a stale block is exactly the problem here, because of inconsistent responses. I think we have a miscommunication here... It sound like you're saying there's no way to avoid working on a stale block...
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361214383)
Working on a stale block is exactly the problem here, because of inconsistent responses. I think we have a miscommunication here... It sound like you're saying there's no way to avoid working on a stale block...
💬 mcelrath commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361224527)
@sipa we're planning on switching to the new IPC mechanism anyway, that StratumV2 is also using. Do you have any comments about synchronicity in that context?
FWIW I think I'm the only person ever to have used ZMQ. My `contrib/zmq/zmq_sub.py` file no longer works due to python async changes. I'm also planning on submitting a PR to update it. But maybe I agree ZMQ might be a mistake. OTOH I think polling in the 21st century is also a mistake.
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361224527)
@sipa we're planning on switching to the new IPC mechanism anyway, that StratumV2 is also using. Do you have any comments about synchronicity in that context?
FWIW I think I'm the only person ever to have used ZMQ. My `contrib/zmq/zmq_sub.py` file no longer works due to python async changes. I'm also planning on submitting a PR to update it. But maybe I agree ZMQ might be a mistake. OTOH I think polling in the 21st century is also a mistake.
💬 stickies-v commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1766980655)
Addressed in #30928.
You're right that the unconditional logging should make these errors quite likely to be caught in review, and that there is no consistency in when we catch formatting exceptions or not. In #30928 I tried to address the latter by enforcing run-time exceptions are handled whenever we use `util::ConstevalFormatString`, which should correlate with needing to prioritize run-time robustness.
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1766980655)
Addressed in #30928.
You're right that the unconditional logging should make these errors quite likely to be caught in review, and that there is no consistency in when we catch formatting exceptions or not. In #30928 I tried to address the latter by enforcing run-time exceptions are handled whenever we use `util::ConstevalFormatString`, which should correlate with needing to prioritize run-time robustness.
💬 l0rinc commented on pull request "build: speed up by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2361230687)
Now that https://github.com/bitcoin/bitcoin/pull/30888 is merged, the difference is not that dramatic: I'm measuring a `0.5%` change:
### latest `master` 2x
`% ccache --clear && git clean -fxd && cmake -B build && time cmake --build build -j10`
> cmake --build build -j10 994.42s user 91.78s system 832% cpu 2:10.47 total
> cmake --build build -j10 993.43s user 91.40s system 832% cpu 2:10.30 total
### rebased branch 2x
`% ccache --clear && git clean -fxd && cmake -B build && time cmake
...
(https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2361230687)
Now that https://github.com/bitcoin/bitcoin/pull/30888 is merged, the difference is not that dramatic: I'm measuring a `0.5%` change:
### latest `master` 2x
`% ccache --clear && git clean -fxd && cmake -B build && time cmake --build build -j10`
> cmake --build build -j10 994.42s user 91.78s system 832% cpu 2:10.47 total
> cmake --build build -j10 993.43s user 91.40s system 832% cpu 2:10.30 total
### rebased branch 2x
`% ccache --clear && git clean -fxd && cmake -B build && time cmake
...
📝 maflcko opened a pull request: " log: Enforce trailing newline at compile time "
(https://github.com/bitcoin/bitcoin/pull/30929)
There are many problems around missing a trailing newline while logging:
* Leaving a line unterminated is racy and can cause content corruption by mixing log lines from different sources.
* It requires extra code like `m_started_new_line` to keep track of, which is annoying and pointless to maintain, because it is currently dead code, see https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1684380835.
* It requires a standalone `unterminated-logprintf` clang-tidy plugin, which is unmain
...
(https://github.com/bitcoin/bitcoin/pull/30929)
There are many problems around missing a trailing newline while logging:
* Leaving a line unterminated is racy and can cause content corruption by mixing log lines from different sources.
* It requires extra code like `m_started_new_line` to keep track of, which is annoying and pointless to maintain, because it is currently dead code, see https://github.com/bitcoin/bitcoin/pull/30386#discussion_r1684380835.
* It requires a standalone `unterminated-logprintf` clang-tidy plugin, which is unmain
...
💬 theStack commented on pull request "test: switch MiniWallet padding unit from weight to vsize":
(https://github.com/bitcoin/bitcoin/pull/30718#discussion_r1766995759)
> without this multiplier was there an issue in this PR?
Yes, without it the assertion failed (`AssertionError: 75000 <= 135680`).
(https://github.com/bitcoin/bitcoin/pull/30718#discussion_r1766995759)
> without this multiplier was there an issue in this PR?
Yes, without it the assertion failed (`AssertionError: 75000 <= 135680`).
💬 mzumsande commented on issue "Race condition between ZMQ UpdateTip and getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361266204)
Would be nice to respond to my prior message: Why doesn't your [Miner log](https://github.com/braidpool/bitcoin/issues/1#issuecomment-2341397203) show a timestamped "[Calling bitcoin-cli](https://github.com/braidpool/bitcoin/blob/29c38babfb83f8520a882bad6b25d1851b7b4a41/contrib/cpunet/miner#L108) (...) getblocktemplate" entry, if, supposedly, the relevant call to `getblocktemplate` call was sent off after the zmq notification for the new tip was received?
Without an explanation for that, it j
...
(https://github.com/bitcoin/bitcoin/issues/30862#issuecomment-2361266204)
Would be nice to respond to my prior message: Why doesn't your [Miner log](https://github.com/braidpool/bitcoin/issues/1#issuecomment-2341397203) show a timestamped "[Calling bitcoin-cli](https://github.com/braidpool/bitcoin/blob/29c38babfb83f8520a882bad6b25d1851b7b4a41/contrib/cpunet/miner#L108) (...) getblocktemplate" entry, if, supposedly, the relevant call to `getblocktemplate` call was sent off after the zmq notification for the new tip was received?
Without an explanation for that, it j
...
👍 tdb3 approved a pull request: "Follow-up after AutoFile position caching: remove unused code"
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315834850)
CR ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369
(https://github.com/bitcoin/bitcoin/pull/30927#pullrequestreview-2315834850)
CR ACK caac06f784c5d94c6a5f7d0b586f9ddbbe55c369