Bitcoin Core Github
44 subscribers
120K links
Download Telegram
πŸ’¬ vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2102279134)
Yes. Added one more commit to handle those.
πŸš€ fanquake merged a pull request: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32292)
πŸ‘ fanquake approved a pull request: "random: Use modern Windows randomness functions"
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2860814033)
ACK 6b4bcc16234575108bb691c15c3532198d9bf98a
βœ… fanquake closed an issue: "Use of deprecated CryptAcquireContext in random.cpp"
(https://github.com/bitcoin/bitcoin/issues/32391)
πŸš€ fanquake merged a pull request: "random: Use modern Windows randomness functions"
(https://github.com/bitcoin/bitcoin/pull/32400)
πŸ’¬ vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2102300215)
> This throw would trigger the Assume() in ~AutoFile() to fail, no?

Yes. Added a `fclose()` call before the `throw` to address that.

I will consider the lambda in the destructor again. From the `fclose()` callers, I counted:

* 7 that ignore the `fclose()` return value
* 7 that log message + take a different code path
* 4 throw

Taking a different code path upon `fclose()` failure would be tricky when that is called from the `AutoFile` destructor. It would require code like:

```cp
...
πŸ’¬ hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2102306083)
> The only tool we need is msbuild.exe which is what my original version did?

And we do use itβ€”along with the VCToolsVersion environment variableβ€”but only to determine whether the vcpkg binary cache needs to be invalidated. CMake is smart enough to inspect the build environment and locate the required tools on its own.

We might be able to use other indicators to invalidate the cache at the right time. Perhaps the top commit hash of vcpkg?
πŸ’¬ TheCharlatan commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2900897714)
Concept ACK
πŸ’¬ maflcko commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout) under GLIBCXX debug mode":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2900903169)
I am out of ideas on how to proceed here. I tried this again on Ubuntu and the gcc-13 failed frequently (https://cirrus-ci.com/task/4846423977492480), whereas the previous commit with gcc-14 passed 100 times (https://cirrus-ci.com/task/5508081775280128)

* I suspect a downgrade from `DEBUG=1` to `-D_GLIBCXX_ASSERTIONS` should also work on centos (c.f. https://cirrus-ci.com/task/5739615073599488)
* Alternatively, we take https://github.com/bitcoin/bitcoin/pull/32529
πŸ“ maflcko opened a pull request: "ci: Downgrade DEBUG=1 to -D_GLIBCXX_ASSERTIONS in centos task"
(https://github.com/bitcoin/bitcoin/pull/32586)
to work around https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2900903169

closes #32524
πŸ’¬ m3dwards commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2102389616)
@hebasto thanks, I see. I will look at using the top commit hash for cache invalidation. If I've understood you correct the current cache key is:

`hashFiles('cmake_version', 'msbuild_version', 'toolset_version', 'vcpkg.json')`

we think this will be sufficient:

`hashFiles('vcpkg_commit_hash', 'vcpkg.json')`

Which then means we can drop the vswhere tool finding.
πŸ’¬ hebasto commented on pull request "ci: remove 3rd party js from windows dll gha job":
(https://github.com/bitcoin/bitcoin/pull/32513#discussion_r2102395677)
> I will look at using the top commit hash for cache invalidation. If I've understood you correct the current cache key is:
>
> `hashFiles('cmake_version', 'msbuild_version', 'toolset_version', 'vcpkg.json')`
>
> we think this will be sufficient:
>
> `hashFiles('vcpkg_commit_hash', 'vcpkg.json')`

Yes, I thin so.

> Which then means we can drop the vswhere tool finding.

Right. However, we still need to locate `mt.exe`.
πŸ’¬ juanmigdr commented on issue "estimateSmartFee error: "Insufficient data or no feerate found"":
(https://github.com/bitcoin/bitcoin/issues/31116#issuecomment-2900990650)
I am currently experiencing this same issue again on testnet and testnet4
πŸ’¬ maflcko commented on pull request "depends: hard-code necessary c(xx)flags rather than setting them per-host":
(https://github.com/bitcoin/bitcoin/pull/32584#issuecomment-2900995568)
> The per-host flag values now represent the mandatory flags that cannot be overridden by the environment. Additionally, these flags (-pipe and -std=xx) will no longer be passed into the CMake build.

Would be nice to explain this with commands to test and observe the difference in behavior. This would make it easier to understand the goals and test them.
πŸ’¬ 1440000bytes commented on pull request "log: Additional compact block logging":
(https://github.com/bitcoin/bitcoin/pull/32582#issuecomment-2901057685)
Concept ACK
πŸ’¬ maflcko commented on issue "Support `Accept` HTTP header in REST API":
(https://github.com/bitcoin/bitcoin/issues/32583#issuecomment-2901076206)
Not sure. This means there will be two conflicting? ways to specify the same. I don't see the benefit either.
πŸ’¬ TheCharlatan commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#issuecomment-2901082433)
@fjahr can you rebase this, I'm still interested in this change.
πŸ’¬ fjahr commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-2901105674)
> Looks like CI failed

Thanks, I had missed that. Seems to have been a silent merge conflict. Kind of weird that only one environment failed but I have come up with an easier way to produce the corrupted db failure case and added some documentation on that as well.
πŸ‘ laanwj approved a pull request: "ci: Downgrade DEBUG=1 to -D_GLIBCXX_ASSERTIONS in centos task"
(https://github.com/bitcoin/bitcoin/pull/32586#pullrequestreview-2861096204)
ACK fa079538e32d20aec6786c93e1117da1e8ea0cab
πŸ’¬ brunoerg commented on pull request "fuzz: doc: add info about `afl-system-config` for macOS":
(https://github.com/bitcoin/bitcoin/pull/32175#issuecomment-2901123554)
> I noticed that a couple lines above, the link to "selecting the best AFL compiler..." is invalid and has instead moved to https://github.com/AFLplusplus/AFLplusplus/blob/stable/docs/fuzzing_in_depth.md#a-selecting-the-best-afl-compiler-for-instrumenting-the-target. I can open a PR to fix the doc link.

Cool, go ahead.