💬 l0rinc commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2424874997)
3087bd1ef1e9f85bf6a0b7c24be650f77a6c030a
I'm finding this commit difficult to review in depth due to its complexity. Like before, I'm seeing the solution (out-of-order processing with task tracking and opportunistic post-processing) before experiencing the problem it solves.
Could we break this down into focused, trivial commits, where low-risk changes are separated from high-risk ones and where the commits tell a story, where the pain is experienced before we propose a solution.
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2424874997)
3087bd1ef1e9f85bf6a0b7c24be650f77a6c030a
I'm finding this commit difficult to review in depth due to its complexity. Like before, I'm seeing the solution (out-of-order processing with task tracking and opportunistic post-processing) before experiencing the problem it solves.
Could we break this down into focused, trivial commits, where low-risk changes are separated from high-risk ones and where the commits tell a story, where the pain is experienced before we propose a solution.
💬 l0rinc commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2424865671)
It seems to me we can loosen the args and avoid string concat:
```suggestion
template <typename T>
void WaitFor(std::span<const std::future<T>> futures, std::string_view context)
{
for (size_t i = 0; i < futures.size(); ++i) {
if (futures[i].wait_for(TIMEOUT_SECS) != std::future_status::ready) {
throw std::runtime_error(strprintf("Timeout waiting for: %s, task index %d", context, i));
}
}
}
```
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2424865671)
It seems to me we can loosen the args and avoid string concat:
```suggestion
template <typename T>
void WaitFor(std::span<const std::future<T>> futures, std::string_view context)
{
for (size_t i = 0; i < futures.size(); ++i) {
if (futures[i].wait_for(TIMEOUT_SECS) != std::future_status::ready) {
throw std::runtime_error(strprintf("Timeout waiting for: %s, task index %d", context, i));
}
}
}
```
💬 stringintech commented on pull request "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation":
(https://github.com/bitcoin/bitcoin/pull/33604#issuecomment-3395390753)
This change allows the snapshot chainstate to reorg to chains that don't include the snapshot block after validation completes (not sure yet if this is the only mechanism that would allow such reorgs), so we must not assume anywhere in the codebase that `SnapshotBase()` is always an ancestor of the active tip. I'll take a closer look to see if any such assumptions exist, but I thought this was worth noting here.
(https://github.com/bitcoin/bitcoin/pull/33604#issuecomment-3395390753)
This change allows the snapshot chainstate to reorg to chains that don't include the snapshot block after validation completes (not sure yet if this is the only mechanism that would allow such reorgs), so we must not assume anywhere in the codebase that `SnapshotBase()` is always an ancestor of the active tip. I'll take a closer look to see if any such assumptions exist, but I thought this was worth noting here.
👍 TheCharlatan approved a pull request: "refactor: inline constant return values from `dbwrapper` write methods"
(https://github.com/bitcoin/bitcoin/pull/33042#pullrequestreview-3329597424)
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
(https://github.com/bitcoin/bitcoin/pull/33042#pullrequestreview-3329597424)
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
⚠️ gmart7t2 opened an issue: "Remove remaining mention of datacarriersize being deprecated"
(https://github.com/bitcoin/bitcoin/issues/33605)
#33453 undeprecated the datacarrier settings, but https://github.com/bitcoin/bitcoin/blob/v30.0/src/qt/bitcoinstrings.cpp#L165 still says:
QT_TRANSLATE_NOOP("bitcoin-core", "Options '-datacarrier' or '-datacarriersize' are set but are marked as deprecated. They will be removed in a future version."),
Best to remove that too?
(https://github.com/bitcoin/bitcoin/issues/33605)
#33453 undeprecated the datacarrier settings, but https://github.com/bitcoin/bitcoin/blob/v30.0/src/qt/bitcoinstrings.cpp#L165 still says:
QT_TRANSLATE_NOOP("bitcoin-core", "Options '-datacarrier' or '-datacarriersize' are set but are marked as deprecated. They will be removed in a future version."),
Best to remove that too?
💬 l0rinc commented on issue "Remove remaining mention of datacarriersize being deprecated":
(https://github.com/bitcoin/bitcoin/issues/33605#issuecomment-3395500448)
this will be removed automatically
(https://github.com/bitcoin/bitcoin/issues/33605#issuecomment-3395500448)
this will be removed automatically
💬 ROLANDSIHOMBING1 commented on pull request "ipc: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3395693550)
Who are you?
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-3395693550)
Who are you?
🤔 ROLANDSIHOMBING1 reviewed a pull request: "doc: bump the template macOS version"
(https://github.com/bitcoin/bitcoin/pull/33573#pullrequestreview-3329865013)
Create
(https://github.com/bitcoin/bitcoin/pull/33573#pullrequestreview-3329865013)
Create
💬 ROLANDSIHOMBING1 commented on pull request "doc: bump the template macOS version":
(https://github.com/bitcoin/bitcoin/pull/33573#discussion_r2425136569)
+ placeholder : e.g." MacOS 26.0 " or
"Ubuntu 26.05 LTS"
(https://github.com/bitcoin/bitcoin/pull/33573#discussion_r2425136569)
+ placeholder : e.g." MacOS 26.0 " or
"Ubuntu 26.05 LTS"
💬 pablomartin4btc commented on pull request "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)":
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3395750632)
-<ins>_**Updates**_</ins>:
- Addressed @ryanofsky's feedback:
- `bitcoin-cli` accepts RPC arguments that begin with a `-` character;
- added a note in description about backwards compatibility;
- `bitcoin-cli` treat options after the RPC method name that begin with double dashes as RPC named parameters;
- Updated description accordingly;
- CentOS CI failure is unrelated (_"no space left on device"_ -> it seems #33293);
- Pending to fix Tidy CI failure (_"recursive call chain"_).
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3395750632)
-<ins>_**Updates**_</ins>:
- Addressed @ryanofsky's feedback:
- `bitcoin-cli` accepts RPC arguments that begin with a `-` character;
- added a note in description about backwards compatibility;
- `bitcoin-cli` treat options after the RPC method name that begin with double dashes as RPC named parameters;
- Updated description accordingly;
- CentOS CI failure is unrelated (_"no space left on device"_ -> it seems #33293);
- Pending to fix Tidy CI failure (_"recursive call chain"_).
💬 pablomartin4btc commented on pull request "argsman, cli: GNU-style command-line option parsing (allows options after non-option arguments)":
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3395916912)
-<ins>_**Updates**_</ins>:
- Fixed previous Tidy CI failure (_"recursive call chain"_), but still working on a refactor of `ArgsManager::ProcessOptionKey`.
(https://github.com/bitcoin/bitcoin/pull/33540#issuecomment-3395916912)
-<ins>_**Updates**_</ins>:
- Fixed previous Tidy CI failure (_"recursive call chain"_), but still working on a refactor of `ArgsManager::ProcessOptionKey`.
💬 maflcko commented on issue "Missing shell completions for the `bitcoin` command":
(https://github.com/bitcoin/bitcoin/issues/33603#issuecomment-3396040984)
See https://github.com/bitcoin/bitcoin/pull/33385?
(https://github.com/bitcoin/bitcoin/issues/33603#issuecomment-3396040984)
See https://github.com/bitcoin/bitcoin/pull/33385?
📝 maflcko opened a pull request: "ci: Work around GitHub rate limit on raw curl usage"
(https://github.com/bitcoin/bitcoin/pull/33606)
(https://github.com/bitcoin/bitcoin/pull/33606)
✅ maflcko closed a pull request: "refactor: Split multithreaded case out of CheckInputScripts"
(https://github.com/bitcoin/bitcoin/pull/32575)
(https://github.com/bitcoin/bitcoin/pull/32575)
📝 maflcko reopened a pull request: "refactor: Split multithreaded case out of CheckInputScripts"
(https://github.com/bitcoin/bitcoin/pull/32575)
This topic has been motivated by my work on batch validation and a related conversation just happened here: https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2098305199
`CheckInputScripts` currently only does what its name implies if there is no multithreading usage for validation. If there are worker threads available the function instead only creates the checks and puts them into a vector. Aside from some shared pre-checks the multithreaded code path is much simpler compared to the
...
(https://github.com/bitcoin/bitcoin/pull/32575)
This topic has been motivated by my work on batch validation and a related conversation just happened here: https://github.com/bitcoin/bitcoin/pull/32467#discussion_r2098305199
`CheckInputScripts` currently only does what its name implies if there is no multithreading usage for validation. If there are worker threads available the function instead only creates the checks and puts them into a vector. Aside from some shared pre-checks the multithreaded code path is much simpler compared to the
...
💬 prusnak commented on pull request "contrib: Add bash completion for new bitcoin command":
(https://github.com/bitcoin/bitcoin/pull/33385#issuecomment-3396086143)
ISTM "bitcoin test" is not covered
(https://github.com/bitcoin/bitcoin/pull/33385#issuecomment-3396086143)
ISTM "bitcoin test" is not covered
💬 maflcko commented on pull request "ci: Use native platform for win-cross task":
(https://github.com/bitcoin/bitcoin/pull/33558#issuecomment-3396095171)
> > * Unlock the CI task to run on riscv64 at all
>
> Isn't this relevant to other tasks as well, for example, `ci/test/00_setup_env_arm.sh`?
That task requires the aarch64 architecture and is not a native task. Not sure if it matters much if qemu-aarch64 is running on x86 or riscv, but feel free to open a separate issue or pull request. The goal here is mainly to speed up the win-cross task for non-x86 arches. Being able to run it on riscv64 is a nice cherry on top.
(https://github.com/bitcoin/bitcoin/pull/33558#issuecomment-3396095171)
> > * Unlock the CI task to run on riscv64 at all
>
> Isn't this relevant to other tasks as well, for example, `ci/test/00_setup_env_arm.sh`?
That task requires the aarch64 architecture and is not a native task. Not sure if it matters much if qemu-aarch64 is running on x86 or riscv, but feel free to open a separate issue or pull request. The goal here is mainly to speed up the win-cross task for non-x86 arches. Being able to run it on riscv64 is a nice cherry on top.
💬 maflcko commented on issue "Compilation failure with Clang SNAPSHOT":
(https://github.com/bitcoin/bitcoin/issues/33539#issuecomment-3396110134)
https://github.com/llvm/llvm-project/issues/163057
(https://github.com/bitcoin/bitcoin/issues/33539#issuecomment-3396110134)
https://github.com/llvm/llvm-project/issues/163057
💬 maflcko commented on pull request "ci: Work around GitHub Recv failure on raw curl usage":
(https://github.com/bitcoin/bitcoin/pull/33606#issuecomment-3396208981)
The issue doesn't happen right now, so I am not sure how to test this pull here. Maybe re-open the next time the issue happens?
(https://github.com/bitcoin/bitcoin/pull/33606#issuecomment-3396208981)
The issue doesn't happen right now, so I am not sure how to test this pull here. Maybe re-open the next time the issue happens?
🤔 janb84 reviewed a pull request: "doc: archive release notes for v30.0"
(https://github.com/bitcoin/bitcoin/pull/33601#pullrequestreview-3330287799)
ACK 8d6e49158e3a1b3215484b5d139c5d8a3fffc4c9
Normal post-release cleanup pr, move the release-notes to the release-notes archive
(https://github.com/bitcoin/bitcoin/pull/33601#pullrequestreview-3330287799)
ACK 8d6e49158e3a1b3215484b5d139c5d8a3fffc4c9
Normal post-release cleanup pr, move the release-notes to the release-notes archive