🚀 glozow merged a pull request: "doc: add missing "testnet4" network string in RPC/init help texts"
(https://github.com/bitcoin/bitcoin/pull/30642)
(https://github.com/bitcoin/bitcoin/pull/30642)
📝 epiccurious opened a pull request: "feat(build): improve dependency error reporting in autogen scripts"
(https://github.com/bitcoin/bitcoin/pull/30646)
Improve error reporting in `autogen.sh` (root directory) and `src/minisketch/autogen.sh` by redirecting the autoreconf dependency check's error message to stderr.
Add check for autoreconf dependency to `src/secp256k1/autogen.sh`.
## Impact
This PR modifies all three autogen build scripts:
1. `autogen.sh` (root directory)
2. `src/minisketch/autogen.sh`
3. `src/secp256k1/autogen.sh`
**`autogen.sh` (root directory) and `src/minisketch/autogen.sh`:**
- No change to the functiona
...
(https://github.com/bitcoin/bitcoin/pull/30646)
Improve error reporting in `autogen.sh` (root directory) and `src/minisketch/autogen.sh` by redirecting the autoreconf dependency check's error message to stderr.
Add check for autoreconf dependency to `src/secp256k1/autogen.sh`.
## Impact
This PR modifies all three autogen build scripts:
1. `autogen.sh` (root directory)
2. `src/minisketch/autogen.sh`
3. `src/secp256k1/autogen.sh`
**`autogen.sh` (root directory) and `src/minisketch/autogen.sh`:**
- No change to the functiona
...
💬 martinsaposnic commented on issue "doc: deduplicate list of chain/network strings in RPC/parameter help texts":
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2286498559)
working on this 🙂
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2286498559)
working on this 🙂
💬 epiccurious commented on pull request "feat(build): improve dependency error reporting in autogen scripts":
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2286499127)
Please let me know if it's preferred to squash the three commits.
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2286499127)
Please let me know if it's preferred to squash the three commits.
💬 0xB10C commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2286505348)
Thanks @mzumsande - [all 11 CI runs with your patch succeed](https://github.com/0xB10C/bitcoin/actions/runs/10359982384/job/28712303493).
This should confirm it's indeed a lifetime issue.
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2286505348)
Thanks @mzumsande - [all 11 CI runs with your patch succeed](https://github.com/0xB10C/bitcoin/actions/runs/10359982384/job/28712303493).
This should confirm it's indeed a lifetime issue.
💬 sipa commented on pull request "feat(build): improve dependency error reporting in autogen scripts":
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2286517592)
The changes to secp256k1 should be done in https://github.com/bitcoin-core/secp256k1, and the changes to minisketch should be done in https://github.com/sipa/minisketch.
I assume the autogen.sh will go away with the imminent cmake migration?
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2286517592)
The changes to secp256k1 should be done in https://github.com/bitcoin-core/secp256k1, and the changes to minisketch should be done in https://github.com/sipa/minisketch.
I assume the autogen.sh will go away with the imminent cmake migration?
📝 epiccurious converted_to_draft a pull request: "feat(build): improve dependency error reporting in autogen scripts"
(https://github.com/bitcoin/bitcoin/pull/30646)
Improve error reporting in `autogen.sh` (root directory) and `src/minisketch/autogen.sh` by redirecting the autoreconf dependency check's error message to stderr.
Add check for autoreconf dependency to `src/secp256k1/autogen.sh`.
## Impact
This PR modifies all three autogen build scripts:
1. `autogen.sh` (root directory)
2. `src/minisketch/autogen.sh`
3. `src/secp256k1/autogen.sh`
**`autogen.sh` (root directory) and `src/minisketch/autogen.sh`:**
- No change to the functiona
...
(https://github.com/bitcoin/bitcoin/pull/30646)
Improve error reporting in `autogen.sh` (root directory) and `src/minisketch/autogen.sh` by redirecting the autoreconf dependency check's error message to stderr.
Add check for autoreconf dependency to `src/secp256k1/autogen.sh`.
## Impact
This PR modifies all three autogen build scripts:
1. `autogen.sh` (root directory)
2. `src/minisketch/autogen.sh`
3. `src/secp256k1/autogen.sh`
**`autogen.sh` (root directory) and `src/minisketch/autogen.sh`:**
- No change to the functiona
...
💬 epiccurious commented on pull request "feat(build): improve dependency error reporting in autogen scripts":
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2286523041)
@sipa understood about secp256k1 and minisketch.
Setting as draft to remove those changes from this PR.
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2286523041)
@sipa understood about secp256k1 and minisketch.
Setting as draft to remove those changes from this PR.
💬 achow101 commented on pull request "Migrate legacy wallets that are not loaded":
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2286529365)
> Could replace it directly for [furszy/bitcoin-core@e8d147e](https://github.com/furszy/bitcoin-core/commit/e8d147eeaacac7d434aad8dbbc5022aadf7d112a). It detects the wallet encryption status automatically regardless of its loaded/unloaded state.
Cherry picked.
Also fixed test failure.
(https://github.com/bitcoin-core/gui/pull/824#issuecomment-2286529365)
> Could replace it directly for [furszy/bitcoin-core@e8d147e](https://github.com/furszy/bitcoin-core/commit/e8d147eeaacac7d434aad8dbbc5022aadf7d112a). It detects the wallet encryption status automatically regardless of its loaded/unloaded state.
Cherry picked.
Also fixed test failure.
💬 LarryRuane commented on pull request "log: expand BCLog::LogFlags (categories) to 64 bits":
(https://github.com/bitcoin/bitcoin/pull/26619#issuecomment-2286539254)
@ryanofsky
> Instead of hardcoding uint64_t throughout this PR, it might make sense to add using CategoryMask = uint64_t
I took this suggestion, thanks. As a test, I defined `CategoryMask` to be `uint32_t`, but the compile failed because of the `ULL` suffix on all those constants (width exceeds 32 bits). So I'm trying to work around that by defining a `mask_bit` constant. (Should it have a name like `MaskBit`? It's only used within the `enum` definition.) I think it makes the code easy to r
...
(https://github.com/bitcoin/bitcoin/pull/26619#issuecomment-2286539254)
@ryanofsky
> Instead of hardcoding uint64_t throughout this PR, it might make sense to add using CategoryMask = uint64_t
I took this suggestion, thanks. As a test, I defined `CategoryMask` to be `uint32_t`, but the compile failed because of the `ULL` suffix on all those constants (width exceeds 32 bits). So I'm trying to work around that by defining a `mask_bit` constant. (Should it have a name like `MaskBit`? It's only used within the `enum` definition.) I think it makes the code easy to r
...
👍 vasild approved a pull request: "depends: Set `CMAKE_SYSTEM_VERSION` for CMake builds"
(https://github.com/bitcoin/bitcoin/pull/30465#pullrequestreview-2235917868)
ACK 5ff8074361 the changes look ok.
I just wonder, why the change to `depends/funcs.mk` is not in `hebasto/cmake-staging`, but is in this PR?
(https://github.com/bitcoin/bitcoin/pull/30465#pullrequestreview-2235917868)
ACK 5ff8074361 the changes look ok.
I just wonder, why the change to `depends/funcs.mk` is not in `hebasto/cmake-staging`, but is in this PR?
👍 vasild approved a pull request: "depends: Amend handling flags environment variables"
(https://github.com/bitcoin/bitcoin/pull/30477#pullrequestreview-2235931261)
ACK d438b501f859befa4f758efcff463c0d647bc033
(https://github.com/bitcoin/bitcoin/pull/30477#pullrequestreview-2235931261)
ACK d438b501f859befa4f758efcff463c0d647bc033
💬 ryanofsky commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2286604343)
I guess another thing I'd like to know is if this is the initial C API, and the implementation is around 3000 lines, and it doesn't handle "transactions, block headers, coins cache, utxo set, meta data, and the mempool", how much bigger do you think it will get if it does cover most of the things you would like it to cover? Like is this 20%, 30%, or 50% of the expected size?
I like the idea of reviewing and merging this PR, and establishing a way to interoperate with rust libraries and extern
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2286604343)
I guess another thing I'd like to know is if this is the initial C API, and the implementation is around 3000 lines, and it doesn't handle "transactions, block headers, coins cache, utxo set, meta data, and the mempool", how much bigger do you think it will get if it does cover most of the things you would like it to cover? Like is this 20%, 30%, or 50% of the expected size?
I like the idea of reviewing and merging this PR, and establishing a way to interoperate with rust libraries and extern
...
👍 vasild approved a pull request: "log: expand BCLog::LogFlags (categories) to 64 bits"
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2236014102)
ACK f64b861c0940ca90869303a9754ebe5bf40bb0c7
I was fine with `(1ULL << 23)` as well. Another way is to use the `0b` prefix (then no need for the `ULL` suffix):
```cpp
enum LogFlags : CategoryMask {
NONE = 0b00000000000000000000000000000000,
NET = 0b00000000000000000000000000000001,
TOR = 0b00000000000000000000000000000010,
MEMPOOL = 0b00000000000000000000000000000100,
HTTP = 0b00000000000000000000000000001000,
....
};
```
(https://github.com/bitcoin/bitcoin/pull/26619#pullrequestreview-2236014102)
ACK f64b861c0940ca90869303a9754ebe5bf40bb0c7
I was fine with `(1ULL << 23)` as well. Another way is to use the `0b` prefix (then no need for the `ULL` suffix):
```cpp
enum LogFlags : CategoryMask {
NONE = 0b00000000000000000000000000000000,
NET = 0b00000000000000000000000000000001,
TOR = 0b00000000000000000000000000000010,
MEMPOOL = 0b00000000000000000000000000000100,
HTTP = 0b00000000000000000000000000001000,
....
};
```
💬 ryanofsky commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1715587191)
I think I would probably add `base_blob::FromUserHex()` function wrapping `base_blob::FromHex()`. `FromHex()` would require strict formatting with fixed width strings and no extraneous characters. But when provided with user input, `FromUserHex()` would allow `0x` prefixes, and not require numbers to be padded to a fixed number of digits. Either way, numbers that are too big to fit in the blob or contained unrecognized characters would be errors.
There is no need for either of these functions
...
(https://github.com/bitcoin/bitcoin/pull/30569#discussion_r1715587191)
I think I would probably add `base_blob::FromUserHex()` function wrapping `base_blob::FromHex()`. `FromHex()` would require strict formatting with fixed width strings and no extraneous characters. But when provided with user input, `FromUserHex()` would allow `0x` prefixes, and not require numbers to be padded to a fixed number of digits. Either way, numbers that are too big to fit in the blob or contained unrecognized characters would be errors.
There is no need for either of these functions
...
💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1715612905)
> [8523d19](https://github.com/bitcoin/bitcoin/commit/8523d19bb3f8c982c6e01a4635c98cd9ac386dea): I wonder if this causes a problem when handling a block template without a witness commitment.
>
> If so, I might just have stratum v2 templates always set a coinbase witness.
This just sets a serialization flag called `allow_witness` and `fAllowWitness` in [`SerializeTransaction`](https://github.com/bitcoin/bitcoin/blob/1a41e63575986887ae34993b4433ec711ae0ffbc/src/primitives/transaction.h#L256
...
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1715612905)
> [8523d19](https://github.com/bitcoin/bitcoin/commit/8523d19bb3f8c982c6e01a4635c98cd9ac386dea): I wonder if this causes a problem when handling a block template without a witness commitment.
>
> If so, I might just have stratum v2 templates always set a coinbase witness.
This just sets a serialization flag called `allow_witness` and `fAllowWitness` in [`SerializeTransaction`](https://github.com/bitcoin/bitcoin/blob/1a41e63575986887ae34993b4433ec711ae0ffbc/src/primitives/transaction.h#L256
...
📝 TheBlueMatt opened a pull request: "Move maximum timewarp attack threshold back to 600s from 7200s"
(https://github.com/bitcoin/bitcoin/pull/30647)
In 6bfa26048dbafb91e9ca63ea8d3960271e798098 the testnet4 timewarp attack fix block time variation was increased from the Great Consensus Cleanup value of 600s to 7200s on the thesis that this allows miners to always create blocks with the current time. Sadly, doing so does allow for some nonzero inflation, even if not a huge amount.
While it could be that some hardware ignores the timestamp provided to it over Stratum and forces the block header timestamp to the current time, I'm not aware of
...
(https://github.com/bitcoin/bitcoin/pull/30647)
In 6bfa26048dbafb91e9ca63ea8d3960271e798098 the testnet4 timewarp attack fix block time variation was increased from the Great Consensus Cleanup value of 600s to 7200s on the thesis that this allows miners to always create blocks with the current time. Sadly, doing so does allow for some nonzero inflation, even if not a huge amount.
While it could be that some hardware ignores the timestamp provided to it over Stratum and forces the block header timestamp to the current time, I'm not aware of
...
💬 agroce commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2286681264)
I got about 20 hours to find running libfuzzer with value profile + length 50, starting from every corpus entry that's 50 or less (seeing if throwing out longer corpus entries is better than truncating, for this kind of thing)
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2286681264)
I got about 20 hours to find running libfuzzer with value profile + length 50, starting from every corpus entry that's 50 or less (seeing if throwing out longer corpus entries is better than truncating, for this kind of thing)
💬 agroce commented on issue "fuzz: `script`: Assertion `!extract_destination_ret' failed.":
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2286681759)
None of my other configs have found it yet...
(https://github.com/bitcoin/bitcoin/issues/30615#issuecomment-2286681759)
None of my other configs have found it yet...
👍 stickies-v approved a pull request: "refactor: Remove Span operator==, Use std::ranges::equal"
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2235986033)
ACK fad0cf6f2619df8df435a2da6da49eeb5510a10f
Even though not having span comparison operators can make things a bit more verbose, these changes are needed to start using `std::span` over our own implementation. This approach seems like the least invasive one (cleaning up wherever possible), and I can't see any behaviour change.
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2235986033)
ACK fad0cf6f2619df8df435a2da6da49eeb5510a10f
Even though not having span comparison operators can make things a bit more verbose, these changes are needed to start using `std::span` over our own implementation. This approach seems like the least invasive one (cleaning up wherever possible), and I can't see any behaviour change.