💬 l0rinc commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993873254)
the problem is likely that the method return `int8_t` for an index - unnecessary, should likely be `size_t` or `int` - not important
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993873254)
the problem is likely that the method return `int8_t` for an index - unnecessary, should likely be `size_t` or `int` - not important
💬 jonatack commented on pull request "CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993885246)
That's true about the index being int.
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993885246)
That's true about the index being int.
🤔 BrandonOdiwuor reviewed a pull request: "contrib: Fix `gen-bitcoin-conf.sh`"
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2682713176)
crACK a24419f8bed5e1145ce171dbbdad957750585471
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2682713176)
crACK a24419f8bed5e1145ce171dbbdad957750585471
👍 dergoegge approved a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2682741320)
ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2682741320)
ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
💬 GarmashAlex commented on pull request "tests: Improve stderr validation in test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/31966#issuecomment-2721999168)
@maflcko could i just create a new PR instead of this one . I don't know how to squash commits
(https://github.com/bitcoin/bitcoin/pull/31966#issuecomment-2721999168)
@maflcko could i just create a new PR instead of this one . I don't know how to squash commits
📝 janb84 opened a pull request: "contrib: Update coverage.cpp macro to support macOS"
(https://github.com/bitcoin/bitcoin/pull/32059)
In PR [#31901](https://github.com/bitcoin/bitcoin/pull/31901), Coverage.cpp was introduced as a separate utility file. However, the macro defined in Coverage.cpp was limited to Clang and Linux, which caused issues for users on macOS when using the newly introduced deterministic test tooling.
This change extends the macro test to include support for the Apple platform, ensuring that macOS users can utilize the coverage functionality so that the deterministic test tooling can run without encoun
...
(https://github.com/bitcoin/bitcoin/pull/32059)
In PR [#31901](https://github.com/bitcoin/bitcoin/pull/31901), Coverage.cpp was introduced as a separate utility file. However, the macro defined in Coverage.cpp was limited to Clang and Linux, which caused issues for users on macOS when using the newly introduced deterministic test tooling.
This change extends the macro test to include support for the Apple platform, ensuring that macOS users can utilize the coverage functionality so that the deterministic test tooling can run without encoun
...
💬 Sjors commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2722033464)
From the original PR it's not obviously how to use this, is there a tl&dr for what to test?
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2722033464)
From the original PR it's not obviously how to use this, is there a tl&dr for what to test?
📝 VolodymyrBg opened a pull request: "test: Add support for mainnet addresses in address_to_scriptpubkey"
(https://github.com/bitcoin/bitcoin/pull/32060)
This commit adds support for mainnet addresses (P2PKH and P2SH) in the address_to_scriptpubkey function in the test framework. Previously, the function only supported testnet addresses and segwit addresses.
The changes include:
- Adding support for mainnet P2PKH addresses (version 0)
- Adding support for mainnet P2SH addresses (version 5)
- Adding tests to verify the new functionality
- Removing the TODO comment as the requested functionality has been implemented
(https://github.com/bitcoin/bitcoin/pull/32060)
This commit adds support for mainnet addresses (P2PKH and P2SH) in the address_to_scriptpubkey function in the test framework. Previously, the function only supported testnet addresses and segwit addresses.
The changes include:
- Adding support for mainnet P2PKH addresses (version 0)
- Adding support for mainnet P2SH addresses (version 5)
- Adding tests to verify the new functionality
- Removing the TODO comment as the requested functionality has been implemented
💬 ryanofsky commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2722056036)
Rebased 7fd7960389254c7a2ba50614e1cbdf8911ef6a7d -> 58f62c2221d82e22e7c2d9cccedd58e4390ace5a ([`pr/cstate.7`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.7) -> [`pr/cstate.8`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.7-rebase..pr/cstate.8)) due to various conflicts.
---
re: https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2720898462
> Shouldn't the block index be our fundamental sourc
...
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2722056036)
Rebased 7fd7960389254c7a2ba50614e1cbdf8911ef6a7d -> 58f62c2221d82e22e7c2d9cccedd58e4390ace5a ([`pr/cstate.7`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.7) -> [`pr/cstate.8`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.8), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.7-rebase..pr/cstate.8)) due to various conflicts.
---
re: https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2720898462
> Shouldn't the block index be our fundamental sourc
...
💬 janb84 commented on pull request "contrib: Update coverage.cpp macro to support macOS":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2722068136)
> From the original PR it's not obvious how to use this, is there a tl&dr for what to test?
Use this config:
```shell
cmake -S . -B build -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'
```
build as normal :
```shell
cmake --build build
```
Run tooling for util_string_tests:
```shell
RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build uti
...
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2722068136)
> From the original PR it's not obvious how to use this, is there a tl&dr for what to test?
Use this config:
```shell
cmake -S . -B build -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DCMAKE_CXX_FLAGS='-fprofile-instr-generate -fcoverage-mapping'
```
build as normal :
```shell
cmake --build build
```
Run tooling for util_string_tests:
```shell
RUST_BACKTRACE=1 cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build uti
...
💬 rkrux commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722093413)
Re: https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2721796758
These are quite subtle points that are raised, but important IMO.
Even if for some reason later it's decided to not do the cpp changes as suggested in the description, the functional test changes as done in this PR are good enough in their own right, and hence, I agree that "part 1" can be dropped from the title unless the author intends to do more type-conversion related functional test changes, which one can argue
...
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722093413)
Re: https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2721796758
These are quite subtle points that are raised, but important IMO.
Even if for some reason later it's decided to not do the cpp changes as suggested in the description, the functional test changes as done in this PR are good enough in their own right, and hence, I agree that "part 1" can be dropped from the title unless the author intends to do more type-conversion related functional test changes, which one can argue
...
💬 instagibbs commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2722117978)
reACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2722117978)
reACK 3c5d1a468199722da620f1f3d8ae3319980a46d5
🤔 marcofleon reviewed a pull request: "fuzz: split `coinselection` harness"
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2683023589)
reACK ba82240553ddd534287845e10bc76b46b45329fe
Only thing that changed since last review is the fix to https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965528993. The order of the two checks has been switched but afaict it shouldn't matter.
(https://github.com/bitcoin/bitcoin/pull/31870#pullrequestreview-2683023589)
reACK ba82240553ddd534287845e10bc76b46b45329fe
Only thing that changed since last review is the fix to https://github.com/bitcoin/bitcoin/pull/31870#discussion_r1965528993. The order of the two checks has been switched but afaict it shouldn't matter.
💬 theStack commented on pull request "test: avoid treating hash results as integers":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722365413)
@ryanofsky @rkrux: Thanks for the feedback! Removed the "(part 1)" from the PR title as suggested and adapted the strong "almost never makes sense" wording in the description. I agree that displaying hash results as byte-reversed hex is an arbitrary decision, but one that stems from the Satoshi-era past and all Bitcoin protocol software that deals with user interface follows it (see also https://bitcoin.stackexchange.com/questions/116730/why-does-bitcoin-core-print-sha256-hashes-uint256-bytes-in
...
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2722365413)
@ryanofsky @rkrux: Thanks for the feedback! Removed the "(part 1)" from the PR title as suggested and adapted the strong "almost never makes sense" wording in the description. I agree that displaying hash results as byte-reversed hex is an arbitrary decision, but one that stems from the Satoshi-era past and all Bitcoin protocol software that deals with user interface follows it (see also https://bitcoin.stackexchange.com/questions/116730/why-does-bitcoin-core-print-sha256-hashes-uint256-bytes-in
...
💬 mzumsande commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1994137293)
ok, makes sense, but I'd challenge that this is "a bug that could cause a fuzz failure" (otherwise I would've expected this failure to be found by now):
My understanding is that in the relevant special case, both `decoded` an `encoded_string` are empty. However, it should be impossible for `encoded_string` to be empty and `decoded` non-empty (and vice versa) because of how base58 works.
(https://github.com/bitcoin/bitcoin/pull/31917#discussion_r1994137293)
ok, makes sense, but I'd challenge that this is "a bug that could cause a fuzz failure" (otherwise I would've expected this failure to be found by now):
My understanding is that in the relevant special case, both `decoded` an `encoded_string` are empty. However, it should be impossible for `encoded_string` to be empty and `decoded` non-empty (and vice versa) because of how base58 works.
💬 mzumsande commented on pull request "fuzz: provide more realistic values to the base58(check) decoders":
(https://github.com/bitcoin/bitcoin/pull/31917#issuecomment-2722394679)
Code Review / lightly tested ACK d5537c18a9034647ba4c9ed4008abd7fee33989e
(https://github.com/bitcoin/bitcoin/pull/31917#issuecomment-2722394679)
Code Review / lightly tested ACK d5537c18a9034647ba4c9ed4008abd7fee33989e
💬 furszy commented on issue "Wallet migration includes rescan":
(https://github.com/bitcoin/bitcoin/issues/32045#issuecomment-2722396445)
> Ideally the migration should finish and then prompt the user that a rescan is about to happen. That way the user can abort the rescan and resume it some other time if it takes too long.
Sadly, we cannot do that right away. The rescan is being triggered by the loading procedure when the wallet is reloaded from disk after migration. We could add a "no scan during init" option (this should also be a new init flag).
> Alternatively, the migrate wallet dialog could at least warn that a rescan is
...
(https://github.com/bitcoin/bitcoin/issues/32045#issuecomment-2722396445)
> Ideally the migration should finish and then prompt the user that a rescan is about to happen. That way the user can abort the rescan and resume it some other time if it takes too long.
Sadly, we cannot do that right away. The rescan is being triggered by the loading procedure when the wallet is reloaded from disk after migration. We could add a "no scan during init" option (this should also be a new init flag).
> Alternatively, the migrate wallet dialog could at least warn that a rescan is
...
🚀 ryanofsky merged a pull request: "wallet: fix crash on double block disconnection"
(https://github.com/bitcoin/bitcoin/pull/31757)
(https://github.com/bitcoin/bitcoin/pull/31757)
💬 hodlinator commented on pull request "qa: Fix TxIndex race conditions":
(https://github.com/bitcoin/bitcoin/pull/32010#issuecomment-2722483103)
> Are the files mentioned in `files_to_delete` and `files_to_perturb` comments just for informational purposes that they don't trigger an error or you want to indicate that these files should trigger an error but currently they don't.
Yes, some of them at least probably should trigger an error, but I'm not knowledgeable of those aspects of LevelDB. It would probably be good to dig deeper into that, but I wanted to keep the focus of this PR on the tx index race conditions.
The reason I touc
...
(https://github.com/bitcoin/bitcoin/pull/32010#issuecomment-2722483103)
> Are the files mentioned in `files_to_delete` and `files_to_perturb` comments just for informational purposes that they don't trigger an error or you want to indicate that these files should trigger an error but currently they don't.
Yes, some of them at least probably should trigger an error, but I'm not knowledgeable of those aspects of LevelDB. It would probably be good to dig deeper into that, but I wanted to keep the focus of this PR on the tx index race conditions.
The reason I touc
...
📝 pinheadmz opened a pull request: "[draft] Replace libevent with our own HTTP and socket-handling implementation"
(https://github.com/bitcoin/bitcoin/pull/32061)
This is a major component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194)
It is based on #30988 but only in the sense that it consumes the `Sockman` class introduced in that PR. The p2p / `Connman` rebase isn't needed for HTTP and therefore this branch could be refactored to only include `sockman.{h,cpp}`.
Commit strategy:
- Assert current behavior of HTTP with additional functional tests, including copying from libevent's tests
- I
...
(https://github.com/bitcoin/bitcoin/pull/32061)
This is a major component of [removing libevent as a dependency of the project](https://github.com/bitcoin/bitcoin/issues/31194)
It is based on #30988 but only in the sense that it consumes the `Sockman` class introduced in that PR. The p2p / `Connman` rebase isn't needed for HTTP and therefore this branch could be refactored to only include `sockman.{h,cpp}`.
Commit strategy:
- Assert current behavior of HTTP with additional functional tests, including copying from libevent's tests
- I
...