Bitcoin Core Github
44 subscribers
121K links
Download Telegram
πŸ’¬ 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
...
πŸ’¬ 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.
πŸ’¬ 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.
πŸ’¬ 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
```
πŸ’¬ 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
...
πŸ’¬ 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
...
⚠️ 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
...
⚠️ 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
...
πŸ’¬ 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.
πŸ’¬ Raimo33 commented on pull request "refactor: make script Solver's often-unused solutions parameter optional":
(https://github.com/bitcoin/bitcoin/pull/33757#issuecomment-3477847760)
> ```c++
> auto SolveType(CScript const& scriptPubKey) -> TxoutType;
> ```

I agree on having a separate method for this. It was my initial approach. It involves some code duplication and adding an extra test but might be worth it to avoid all the `if (vSolutionsRet)` checks.

> ```c++
> auto Solve(CScript const& scriptPubKey) -> std::tuple<TxoutType, std::vector<std::vector<unsigned char>>>;
> ```

Wouldn't this increase allocs/copies? I'd say it's perfectly fine to leave it as is w
...
βœ… fanquake closed an issue: "[Vulnerability Coordination] Bitcoin Core Supply-Chain / CI-CD Integrity Issue – VU#237485"
(https://github.com/bitcoin/bitcoin/issues/33763)
πŸ’¬ fanquake commented on issue "[Vulnerability Coordination] Bitcoin Core Supply-Chain / CI-CD Integrity Issue – VU#237485":
(https://github.com/bitcoin/bitcoin/issues/33763#issuecomment-3477848117)
> and provide a secure intake channel (PGP key/fingerprint

See https://bitcoincore.org/en/contact/. You can send an email to `security@bitcoincore.org` using one of the keys listed there.
πŸ’¬ fanquake commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3477855579)
> not yet tested with a server running pre-v26 (before getaddrmaninfo)

`25.x`, `26.x` and `27.x` are end-of-life? So that seems out of scope for needing to test / accomodate code-wise?
πŸ€” TheCharlatan reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3408737974)
Thanks for the review @stringintech!

Updated e9f14a07ed8b8161840d739c24603539119ee5fd -> e95efc00842d5d0df96ee9294cdf818741be539e ([kernelApi_79](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_79) -> [kernelApi_80](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_80), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_79..kernelApi_80))

* Addressed @stringintech's [comment](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2483514108), simplified chec
...
πŸ’¬ TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2484755640)
I made `GetBlockTreeEntry` an optional now. The developer might not have a way to check inclusion of a certain block hash, so returning an optional seems better. For `GetByHeight`, I would keep the throw, because it is a bit easier to directly plug into the range, and because we have a way for the programmer to guard against bad height values by checking it against the `Height()`.
πŸ’¬ sipa commented on pull request "cli: rework -addrinfo cli to use addresses which aren’t filtered for quality/recency":
(https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-3477872095)
Concept ACK, but I think it's weird to use a long English phrase as field name. Maybe just "addresses_total" and "addresses_filtered" or so? If we care about backward compatibility, we could add a `-deprecatedrpc=addresses_known` option to enable the "addresses_known" field (which would be a copy of "addresses_filtered").
πŸ’¬ sipa commented on pull request "refactor: make script Solver's often-unused solutions parameter optional":
(https://github.com/bitcoin/bitcoin/pull/33757#issuecomment-3477874632)
If we're going to touch this code, my preference would be to go with @purpleKarrot's approach of two separate functions, moving the `vSolutions` field into a return pair element.

If the allocation overhead of that is a concern, I think the proper solution is a follow-up to get rid of the `vSolutions` approach of encoding things, and instead introduce a proper type that encodes it more usefully (possibly an `std::variant`, like `CTxDestination`, but with more possibilities). For multisig-like
...
πŸ€” sipa reviewed a pull request: "refactor: inline constant return values from `dbwrapper` write methods"
(https://github.com/bitcoin/bitcoin/pull/33042#pullrequestreview-3408745524)
ACK 743abbcbde9e5a2db489bca461c98df461eff7d0
πŸ’¬ stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3477890834)
ACK e95efc00
πŸ’¬ achow101 commented on issue "Download links on bitcoincore.org not working":
(https://github.com/bitcoin/bitcoin/issues/33762#issuecomment-3477997646)
> clicking the download buttons returns an error

What is the error?