📝 maflcko converted_to_draft a pull request: "rest: Reject negative outpoint index early in getutxos parsing"
(https://github.com/bitcoin/bitcoin/pull/30444)
In `rest_getutxos` outpoint indexes such as `+N` or `-N` are accepted. This should be harmless, because any index out of range should be treated as a non-existent utxo. However, a negative index can't exist ever, so it seems better to reject all signs, whether `+` or `-`.
(https://github.com/bitcoin/bitcoin/pull/30444)
In `rest_getutxos` outpoint indexes such as `+N` or `-N` are accepted. This should be harmless, because any index out of range should be treated as a non-existent utxo. However, a negative index can't exist ever, so it seems better to reject all signs, whether `+` or `-`.
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1678947516)
It is probably fine to rework the error strings in `rest.cpp`, but seems better to leave for a follow-up, given that there are other follow-ups as well: https://github.com/bitcoin/bitcoin/pull/30444#issuecomment-2227109971 ?
This way, the changes here would be focussed on one thing (fixing the possible UB).
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1678947516)
It is probably fine to rework the error strings in `rest.cpp`, but seems better to leave for a follow-up, given that there are other follow-ups as well: https://github.com/bitcoin/bitcoin/pull/30444#issuecomment-2227109971 ?
This way, the changes here would be focussed on one thing (fixing the possible UB).
💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1678963168)
See above https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1660679371, just omitting the word "pool" because in theory there could be something _other_ than a pool that wants to add data to our coinbase, just seems confusing to me.
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1678963168)
See above https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1660679371, just omitting the word "pool" because in theory there could be something _other_ than a pool that wants to add data to our coinbase, just seems confusing to me.
💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1678967962)
#29432 implements this in more detail. I'm not even 100% sure if it's correct there, so I'd rather wait with documentation the exact numbers - that might just encourage someone to recompile and burn themselves.
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1678967962)
#29432 implements this in more detail. I'm not even 100% sure if it's correct there, so I'd rather wait with documentation the exact numbers - that might just encourage someone to recompile and burn themselves.
💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1678972145)
`MAX_STANDARD_TX_WEIGHT` is a standardness rule. If we wanted to enforce it in the mining code, which would be unrelated to this PR, it shouldn't be through an assert / assume.
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1678972145)
`MAX_STANDARD_TX_WEIGHT` is a standardness rule. If we wanted to enforce it in the mining code, which would be unrelated to this PR, it shouldn't be through an assert / assume.
💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1678979204)
No, I think the `LOCK(m_tx_download_mutex)` already checks that it isn't already held, since it's a `Mutex`.
I guess this seems a bit random, but I'm trying to establish the lock hierarchy as `m_tx_download_mutex`, then `m_mempool.cs` (which is a `RecursiveMutex`). That's also why we can only fire `ActiveTipChanged` after releasing the mempool mutex. Later code added to `TxDownloadManager` will call `CTxMemPool` functions that take the mempool lock (see e.g. https://github.com/bitcoin/bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1678979204)
No, I think the `LOCK(m_tx_download_mutex)` already checks that it isn't already held, since it's a `Mutex`.
I guess this seems a bit random, but I'm trying to establish the lock hierarchy as `m_tx_download_mutex`, then `m_mempool.cs` (which is a `RecursiveMutex`). That's also why we can only fire `ActiveTipChanged` after releasing the mempool mutex. Later code added to `TxDownloadManager` will call `CTxMemPool` functions that take the mempool lock (see e.g. https://github.com/bitcoin/bitcoin
...
💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2230320294)
I took @ryanofsky's patch and split it into two commits.
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2230320294)
I took @ryanofsky's patch and split it into two commits.
⚠️ 1alexbc1 opened an issue: "Bugreport"
(https://github.com/bitcoin-core/gui/issues/829)
### Please describe the feature you'd like to see added.
Bug
### Is your feature related to a problem, if so please describe it.
Bug
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
_No response_
### Please leave any additional context
_No response_
(https://github.com/bitcoin-core/gui/issues/829)
### Please describe the feature you'd like to see added.
Bug
### Is your feature related to a problem, if so please describe it.
Bug
### Describe the solution you'd like
_No response_
### Describe any alternatives you've considered
_No response_
### Please leave any additional context
_No response_
✅ fanquake closed an issue: "Bugreport"
(https://github.com/bitcoin-core/gui/issues/829)
(https://github.com/bitcoin-core/gui/issues/829)
✅ glozow closed an issue: "ci: failure in p2p_handshake.py"
(https://github.com/bitcoin/bitcoin/issues/30368)
(https://github.com/bitcoin/bitcoin/issues/30368)
🚀 glozow merged a pull request: "net: fix race condition in self-connect detection"
(https://github.com/bitcoin/bitcoin/pull/30394)
(https://github.com/bitcoin/bitcoin/pull/30394)
👍 fanquake approved a pull request: "contrib: use c++ compiler rather than c compiler for binary checks"
(https://github.com/bitcoin/bitcoin/pull/30387#pullrequestreview-2179613590)
ACK 9010b1343b9f931f771d3d49dd03b57868c24d5d
Guix Build (aarch64):
```bash
77365de6a5a0dd40d79f2c4ba8ac8f105c44348076ef64eb36c9ae030ce833f8 guix-build-9010b1343b9f/output/aarch64-linux-gnu/SHA256SUMS.part
af1b2f2cd45ce3b510903c674d68545e0ec0ad4cb5b7e65b7e6e8f17eabb4f4f guix-build-9010b1343b9f/output/aarch64-linux-gnu/bitcoin-9010b1343b9f-aarch64-linux-gnu-debug.tar.gz
a88c00e179e1302a3ffd438c175b2f6331b58f0484757c1f843ca2d2a386fd59 guix-build-9010b1343b9f/output/aarch64-linux-gnu/bitco
...
(https://github.com/bitcoin/bitcoin/pull/30387#pullrequestreview-2179613590)
ACK 9010b1343b9f931f771d3d49dd03b57868c24d5d
Guix Build (aarch64):
```bash
77365de6a5a0dd40d79f2c4ba8ac8f105c44348076ef64eb36c9ae030ce833f8 guix-build-9010b1343b9f/output/aarch64-linux-gnu/SHA256SUMS.part
af1b2f2cd45ce3b510903c674d68545e0ec0ad4cb5b7e65b7e6e8f17eabb4f4f guix-build-9010b1343b9f/output/aarch64-linux-gnu/bitcoin-9010b1343b9f-aarch64-linux-gnu-debug.tar.gz
a88c00e179e1302a3ffd438c175b2f6331b58f0484757c1f843ca2d2a386fd59 guix-build-9010b1343b9f/output/aarch64-linux-gnu/bitco
...
🚀 fanquake merged a pull request: "contrib: use c++ compiler rather than c compiler for binary checks"
(https://github.com/bitcoin/bitcoin/pull/30387)
(https://github.com/bitcoin/bitcoin/pull/30387)
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1679008780)
Updated to avoid changing existing error strings. Also reordered commits, let me know if you prefer I break off the last 2 into a separate PR.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1679008780)
Updated to avoid changing existing error strings. Also reordered commits, let me know if you prefer I break off the last 2 into a separate PR.
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2230367028)
I come bearing good news: the benchmarks have finished running!
https://github.com/bitcoin/bitcoin/pull/28280/commits/0383defd049fa1feb6de7abea259041487ccda1d
```python
Time (mean ± σ): 44949.012 s ± 425.373 s [User: 42783.944 s, System: 9712.690 s]
Range (min … max): 44648.228 s … 45249.796 s 2 runs
```
https://github.com/bitcoin/bitcoin/pull/28280/commits/21090d032f5c4d90a41c3ca2030690c75899ec1a
```python
Time (mean ± σ): 37726.687 s ± 681.782 s [User: 35967.638 s,
...
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2230367028)
I come bearing good news: the benchmarks have finished running!
https://github.com/bitcoin/bitcoin/pull/28280/commits/0383defd049fa1feb6de7abea259041487ccda1d
```python
Time (mean ± σ): 44949.012 s ± 425.373 s [User: 42783.944 s, System: 9712.690 s]
Range (min … max): 44648.228 s … 45249.796 s 2 runs
```
https://github.com/bitcoin/bitcoin/pull/28280/commits/21090d032f5c4d90a41c3ca2030690c75899ec1a
```python
Time (mean ± σ): 37726.687 s ± 681.782 s [User: 35967.638 s,
...
💬 maflcko commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1679027950)
Yeah, I'd say a separate PR would be better.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1679027950)
Yeah, I'd say a separate PR would be better.
💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters once per tip change":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2230386942)
- Updated the PR title and description, thanks @mzumsande
- Rebased for #30394
- Renamed variable https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1665560023
- Added some more documentation to `m_tx_download_mutex` to address https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1678336506.
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2230386942)
- Updated the PR title and description, thanks @mzumsande
- Rebased for #30394
- Renamed variable https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1665560023
- Added some more documentation to `m_tx_download_mutex` to address https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1678336506.
💬 fjahr commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#issuecomment-2230388708)
> It would be nice to implement [81638f5](https://github.com/bitcoin/bitcoin/commit/81638f5d42b841fb327393974320ca47db39eb09) differently, without adding the `<chain.h>` dependency to all index headers. This dependency provides access to node internal types that indexes running on other processes (in the future) will not know about.
Should not be too complicated to get rid of the dependency. I will draft something, maybe I'll just give @maflcko s [suggestion](https://github.com/bitcoin/bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/29770#issuecomment-2230388708)
> It would be nice to implement [81638f5](https://github.com/bitcoin/bitcoin/commit/81638f5d42b841fb327393974320ca47db39eb09) differently, without adding the `<chain.h>` dependency to all index headers. This dependency provides access to node internal types that indexes running on other processes (in the future) will not know about.
Should not be too complicated to get rid of the dependency. I will draft something, maybe I'll just give @maflcko s [suggestion](https://github.com/bitcoin/bitcoi
...
💬 hodlinator commented on pull request "fix: Make TxidFromString() respect string_view length":
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1679040564)
Dropped 2 commits from this PR.
(https://github.com/bitcoin/bitcoin/pull/30436#discussion_r1679040564)
Dropped 2 commits from this PR.
💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters once per tip change":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1679055771)
Ah I was wrong about `LOCK` doing the assert, added that as well.
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1679055771)
Ah I was wrong about `LOCK` doing the assert, added that as well.