💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483975804)
> we would need to get rid of the custom allocator
I'm not so sure, I'd have different dbcache sizes during IBD and only reallocate after it finishes, since we don't actually need to narrow the cache size during IBD. After your input fetcher change we may never need to narrow it. We'll see, let's discuss that separately.
> The non-reuse case should be the only one where we have to be explicit.
I explicitly wanted to avoid that, but let me know if you insist, I don't mind that much.
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483975804)
> we would need to get rid of the custom allocator
I'm not so sure, I'd have different dbcache sizes during IBD and only reallocate after it finishes, since we don't actually need to narrow the cache size during IBD. After your input fetcher change we may never need to narrow it. We'll see, let's discuss that separately.
> The non-reuse case should be the only one where we have to be explicit.
I explicitly wanted to avoid that, but let me know if you insist, I don't mind that much.
💬 oren-z0 commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2483977816)
Oh, now I get it.
So the question is why `tx_verbosity` was required in the first place even when the data-format was binary or hex (where it is ignored).
Consider splitting this into two commits, one to make `tx_verbosity` optional, and another to add the new blockpart feature.
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2483977816)
Oh, now I get it.
So the question is why `tx_verbosity` was required in the first place even when the data-format was binary or hex (where it is ignored).
Consider splitting this into two commits, one to make `tx_verbosity` optional, and another to add the new blockpart feature.
💬 andrewtoth commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483977917)
> I explicitly wanted to avoid that, but let me know if you insist, I don't mind that much.
Don't you think it's safer to have the case that won't break either way be default, and then the performant case that can break if you pick the wrong option be explicit?
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483977917)
> I explicitly wanted to avoid that, but let me know if you insist, I don't mind that much.
Don't you think it's safer to have the case that won't break either way be default, and then the performant case that can break if you pick the wrong option be explicit?
💬 l0rinc commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483978998)
Maybe, not sure, but I took your advice and pushed.
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483978998)
Maybe, not sure, but I took your advice and pushed.
💬 andrewtoth commented on pull request "[IBD] coins: reduce lookups in dbcache layer propagation":
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483980214)
Ahh right, we handle this case even though it can't be hit in production.
I suppose it's valuable for regression testing, so we throw instead of silently allowing this.
(https://github.com/bitcoin/bitcoin/pull/33602#discussion_r2483980214)
Ahh right, we handle this case even though it can't be hit in production.
I suppose it's valuable for regression testing, so we throw instead of silently allowing this.
🤔 stringintech reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3407306489)
Comments on last 8 commits (dcb8d495 through e9f14a07).
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3407306489)
Comments on last 8 commits (dcb8d495 through e9f14a07).
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483514108)
nit
```suggestion
if (m_expected_valid_block.has_value()) {
auto ser_block{block.ToBytes()};
check_equal(m_expected_valid_block.value(), ser_block);
}
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483514108)
nit
```suggestion
if (m_expected_valid_block.has_value()) {
auto ser_block{block.ToBytes()};
check_equal(m_expected_valid_block.value(), ser_block);
}
```
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483940512)
nit
```suggestion
Logger logger{std::make_unique<KernelLog>()};
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483940512)
nit
```suggestion
Logger logger{std::make_unique<KernelLog>()};
```
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483695548)
If the chain reference we get here remains a valid pointer to the best chain even in the case of reorgs (i'm ignoring the more unlikely case where the active chainstate could change), maybe the doc can be a bit more clear:
```cpp
/**
* @brief Returns the best known currently active chain. Its lifetime is
* dependent on the chainstate manager. It can be thought of as a view on a
* vector of block tree entries that form the best chain. The returned chain
* reference always points to the
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483695548)
If the chain reference we get here remains a valid pointer to the best chain even in the case of reorgs (i'm ignoring the more unlikely case where the active chainstate could change), maybe the doc can be a bit more clear:
```cpp
/**
* @brief Returns the best known currently active chain. Its lifetime is
* dependent on the chainstate manager. It can be thought of as a view on a
* vector of block tree entries that form the best chain. The returned chain
* reference always points to the
...
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483939969)
nit
```suggestion
Logger logger{std::make_unique<TestLog>()};
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483939969)
nit
```suggestion
Logger logger{std::make_unique<TestLog>()};
```
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483614770)
If it is possible to have another thread extending/changing the best chain, then i think these three functions should hold `cs_main`. For example all three are calling `size()` on the underlying chain vector.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483614770)
If it is possible to have another thread extending/changing the best chain, then i think these three functions should hold `cs_main`. For example all three are calling `size()` on the underlying chain vector.
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483701019)
The test section 911-934 could become a bit easier to follow if it is broken into multiple parts (just some newlines might be enough):
<details>
<summary>diff</summary>
```diff
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index e2c14b57eb..f78f346711 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -908,25 +908,34 @@ BOOST_AUTO_TEST_CASE(btck_chainman_regtest_tests)
}
}
+ // Read spent outputs
...
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483701019)
The test section 911-934 could become a bit easier to follow if it is broken into multiple parts (just some newlines might be enough):
<details>
<summary>diff</summary>
```diff
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index e2c14b57eb..f78f346711 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -908,25 +908,34 @@ BOOST_AUTO_TEST_CASE(btck_chainman_regtest_tests)
}
}
+ // Read spent outputs
...
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483756743)
Not sure of this but could we return an optional here instead of throwing? (for the same reason that the assertion was removed from `btck_chain_get_by_height()` impl)
Also for `GetBlockTreeEntry()` that implicitly throws in `BlockTreeEntry` ctor now.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483756743)
Not sure of this but could we return an optional here instead of throwing? (for the same reason that the assertion was removed from `btck_chain_get_by_height()` impl)
Also for `GetBlockTreeEntry()` that implicitly throws in `BlockTreeEntry` ctor now.
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483950011)
Looks like the change to `bitcoinkernel.cpp` in the last commit (e9f14a07) should have been included in a different commit.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483950011)
Looks like the change to `bitcoinkernel.cpp` in the last commit (e9f14a07) should have been included in a different commit.
💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483977648)
Better to drop block manager now that we have not exposed it.
```suggestion
* @brief Triggers the start of a reindex if the wipe options were previously set for
* the chainstate manager. Can also import an array of existing block
```
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483977648)
Better to drop block manager now that we have not exposed it.
```suggestion
* @brief Triggers the start of a reindex if the wipe options were previously set for
* the chainstate manager. Can also import an array of existing block
```
💬 151henry151 commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3476949593)
> Thanks. However you should do a build for all `HOSTS`. As this is changing code that effects all `HOSTS`.
Thanks for the guidance.
Guix build for 1e6aa74b4b628e8acafe6a3d2aaf2a6aa0d9d290:
046665733b06b855c98198d8fef9fe5d158f67f4dcbc15000cde0b1cc7a678d5 arm64-apple-darwin/bitcoin-1e6aa74b4b62-arm64-apple-darwin-unsigned.zip
04a35a19a5351a70e3dd66159e2cea6d8ac4a6e20f3cd0423d7b812be9b93ff5 arm64-apple-darwin/bitcoin-1e6aa74b4b62-arm64-apple-darwin-unsigned.tar.gz
09bd1e944d37759c1f52
...
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3476949593)
> Thanks. However you should do a build for all `HOSTS`. As this is changing code that effects all `HOSTS`.
Thanks for the guidance.
Guix build for 1e6aa74b4b628e8acafe6a3d2aaf2a6aa0d9d290:
046665733b06b855c98198d8fef9fe5d158f67f4dcbc15000cde0b1cc7a678d5 arm64-apple-darwin/bitcoin-1e6aa74b4b62-arm64-apple-darwin-unsigned.zip
04a35a19a5351a70e3dd66159e2cea6d8ac4a6e20f3cd0423d7b812be9b93ff5 arm64-apple-darwin/bitcoin-1e6aa74b4b62-arm64-apple-darwin-unsigned.tar.gz
09bd1e944d37759c1f52
...
💬 purpleKarrot commented on pull request "refactor: make script Solver's often-unused solutions parameter optional":
(https://github.com/bitcoin/bitcoin/pull/33757#issuecomment-3477481741)
The proposed approach increases complexity both at the callsites and in the implementation of the function itself.
All the added `&` at the callsites would be unnecessary, if the PR simply added a single argument overload to the `Solver` function instead of a default argument.
Further, `vSolutionsRet` actually seems to be an *output argument*, not an inout argument. Passing that as the return type improves local reasoning:
```cpp
auto SolveType(CScript const& scriptPubKey) -> TxoutType
...
(https://github.com/bitcoin/bitcoin/pull/33757#issuecomment-3477481741)
The proposed approach increases complexity both at the callsites and in the implementation of the function itself.
All the added `&` at the callsites would be unnecessary, if the PR simply added a single argument overload to the `Solver` function instead of a default argument.
Further, `vSolutionsRet` actually seems to be an *output argument*, not an inout argument. Passing that as the return type improves local reasoning:
```cpp
auto SolveType(CScript const& scriptPubKey) -> TxoutType
...
⚠️ bronsii opened an issue: "Download links on bitcoincore.org not working"
(https://github.com/bitcoin/bitcoin/issues/33762)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Hi,
the download links for Bitcoin Core on bitcoincore.org are currently not working. The page loads, but clicking the download buttons returns an error and the download does not start. I have tried multiple browsers (Firefox on Linux) and the issue persists. Please help.
Sorry for posting this here, but I didn’t know where else to ask for help.
Thanks!
### Expected behaviour
I expecte
...
(https://github.com/bitcoin/bitcoin/issues/33762)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Hi,
the download links for Bitcoin Core on bitcoincore.org are currently not working. The page loads, but clicking the download buttons returns an error and the download does not start. I have tried multiple browsers (Firefox on Linux) and the issue persists. Please help.
Sorry for posting this here, but I didn’t know where else to ask for help.
Thanks!
### Expected behaviour
I expecte
...
⚠️ starixapp opened an issue: "[Vulnerability Coordination] Bitcoin Core Supply-Chain / CI-CD Integrity Issue – VU#237485"
(https://github.com/bitcoin/bitcoin/issues/33763)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Hello,
I'm Alex Morgan from Sentinel Core. I have reported a supply-chain/CI-CD integrity issue that affects distributed Bitcoin Core binaries and requires technical coordination.
For context: KuCoin has indicated this falls within Bitcoin Core’s scope, and CERT/CC has opened a coordination case (VU#237485).
Please confirm receipt and provide a secure intake channel (PGP key/fingerprint
...
(https://github.com/bitcoin/bitcoin/issues/33763)
### Is there an existing issue for this?
- [x] I have searched the existing issues
### Current behaviour
Hello,
I'm Alex Morgan from Sentinel Core. I have reported a supply-chain/CI-CD integrity issue that affects distributed Bitcoin Core binaries and requires technical coordination.
For context: KuCoin has indicated this falls within Bitcoin Core’s scope, and CERT/CC has opened a coordination case (VU#237485).
Please confirm receipt and provide a secure intake channel (PGP key/fingerprint
...
💬 starixapp commented on issue "[Vulnerability Coordination] Bitcoin Core Supply-Chain / CI-CD Integrity Issue – VU#237485":
(https://github.com/bitcoin/bitcoin/issues/33763#issuecomment-3477815293)
Requesting coordination: CERT/CC has opened case VU#237485 and KuCoin's security team has indicated this falls within Bitcoin Core's scope. Please confirm a secure intake channel (PGP fingerprint or secure upload endpoint) and a technical contact so we can provide a sanitized teaser for triage.
(https://github.com/bitcoin/bitcoin/issues/33763#issuecomment-3477815293)
Requesting coordination: CERT/CC has opened case VU#237485 and KuCoin's security team has indicated this falls within Bitcoin Core's scope. Please confirm a secure intake channel (PGP fingerprint or secure upload endpoint) and a technical contact so we can provide a sanitized teaser for triage.