Bitcoin Core Github
44 subscribers
121K links
Download Telegram
👍 willcl-ark approved a pull request: "contrib: Fixup verify-binaries OS platform parsing"
(https://github.com/bitcoin/bitcoin/pull/30147#pullrequestreview-2087672777)
tACK 8135632c33f4bad8434403b5807c48d7dba3b3d7

This works much better than the current verifier, and being able to specify an exact version to download is more convenient in general, and especially in places such as [building docker images](https://github.com/willcl-ark/bitcoin-core-docker/blob/803e20df0f2f70ca6a635f378590e6220de743b3/27/Dockerfile#L25-L27) where downloading multiple unneeded versions is purely wasteful.

Still one open comment regarding specifying bitcoincore.org, but happy
...
💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2139083195)
Rebased to resolve a conflict with bitcoin/bitcoin#30049.
💬 laanwj commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1620295387)
SGTM.

> That address_family is the first argument to the socket(2) syscall. What's needed is to extend CreateSock*() with the other two arguments - type and protocol. I can do that.

Yes, for specifying UDP (and NETLINK, but i do not intend to make a test framework for that) it would need all three arguments.
💬 darosior commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2139099415)
Alright so i think we can still pinpoint this issue to avoid having to limit the size of the input. In the same way we limit the derivation paths depth, we could limit the number of sub-fragments per fragment in the input:
```cpp
bool HasLargeFrag(const FuzzBufferType& buff, const int max_subs)
{
std::stack<int> counts;
for (const auto& ch: buff) {
if (ch == '(') {
counts.push(1);
} else if (ch == ',' && !counts.empty()) {
if (++counts.top
...
💬 vasild commented on pull request "init: fixes file descriptor accounting":
(https://github.com/bitcoin/bitcoin/pull/30065#discussion_r1620315609)
If the node is connected to 8 manually defined outbound peers and 0 inbounds and 0 automatic outbounds, then it is still operational? No need to prohibit startup in this case?
💬 marcofleon commented on pull request "fuzz: increase `txorphan` harness stability":
(https://github.com/bitcoin/bitcoin/pull/30186#discussion_r1620315667)
Got it, thanks. Should be fixed now.
💬 marcofleon commented on pull request "fuzz: increase `txorphan` harness stability":
(https://github.com/bitcoin/bitcoin/pull/30186#issuecomment-2139126273)
> You'll have to install and use gnu-sed on macos, or use Linux. If you don't want to test locally and want to know the syntax without testing, you can look at previous examples via `git log --grep='sed -i'`.

I installed gnu-sed and used that locally. CI looks good now.
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2139129818)
I dropped 3559085dcc9ee687c347383a0c2a63f3ea16f001 and 3f4c1d1e515a840c4b119e3ba65a8391d4fc7f6b which introduced `common/transport.h`. It's difficult to maintain such a large diff and it's currently causing a link error. Here's a branch with both of them: https://github.com/Sjors/bitcoin/tree/2024/05/transport

I'd still like to move Transport out of `libbitcoin_node` into `libbitcoin_common`, but this is probably better done in a parallel PR.

At the request of people testing Stratum v2 I'm
...
💬 m3dwards commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#discussion_r1620348574)
I have been hitting issues on github actions runners cancelling the job after 4 minutes ,which is during compile, so this was an attempt to reduce the CPU load. It hasn't helped. See https://github.com/actions/runner-images/issues/9848#issuecomment-2137525256
👍 dergoegge approved a pull request: "fuzz: increase `txorphan` harness stability"
(https://github.com/bitcoin/bitcoin/pull/30186#pullrequestreview-2087781014)
Code review ACK 8defc182a31d828ad0f53ebf7e3be9e9cfc42d09
🤔 cbergqvist reviewed a pull request: "indexes: Don't wipe indexes again when continuing a prior reindex"
(https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2087711102)
Tried cherry-picking only eeea0818c1a20adc5225b98b185953d386c033e0 on top of the base of the PR (058af75874ffa2b4064e3d6d30cc50f0ec754ba8) and ran the test and it succeeds without the other C++ changes?
💬 cbergqvist commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1620305534)
nit: How about taking the opportunity to nail down the mutability of `ChainstateLoadOptions` for improved readability?
```suggestion
const node::ChainstateLoadOptions options{
.mempool = Assert(node.mempool.get()),
.reindex = blockman_opts.reindex,
.reindex_chainstate = fReindexChainState,
.prune = chainman.m_blockman.IsPruneMode(),
.require_full_verification = args.IsArgSet("-checkblocks") || args.IsArgSet("-checklevel")
...
💬 fanquake commented on pull request "build: remove `--enable-lcov-branch-coverage`":
(https://github.com/bitcoin/bitcoin/pull/30192#issuecomment-2139204269)
Updated, rebased, and re-tested, also with LCOV 2.1 (built from source) (updated PR description).
💬 0xB10C commented on pull request "ci: move ASAN job to GitHub Actions from Cirrus CI":
(https://github.com/bitcoin/bitcoin/pull/30193#issuecomment-2139206624)
Concept ACK! Thanks for working on this!
💬 darosior commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2139268235)
> [clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt](https://github.com/bitcoin/bitcoin/files/13877884/clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt)
> [clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt](https://github.com/bitcoin/bitcoin/files/15238132/clusterfuzz-testcase-minimized-descriptor_parse-5888623437217792.bin.txt)

For those there is probably something to be done by someone more experienced at C++ than i a
...
💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2139287173)
> I think we'll have to limit the size of the input, unfortunately.

Yes, as long as all possible real-world scenarios are covered, this is fine as well.
💬 Numbers0689 commented on issue "test: replace bare asserts with assertion helpers (assert_equal() etc.)":
(https://github.com/bitcoin/bitcoin/issues/23119#issuecomment-2139291697)
is the issue still relevant? can i work on it?
💬 maflcko commented on pull request "fuzz: increase `txorphan` harness stability":
(https://github.com/bitcoin/bitcoin/pull/30186#issuecomment-2139292306)
utACK 8defc182a31d828ad0f53ebf7e3be9e9cfc42d09
💬 maflcko commented on pull request "fuzz: increase `txorphan` harness stability":
(https://github.com/bitcoin/bitcoin/pull/30186#issuecomment-2139293546)
cc @glozow This conflicts with some of your pulls, so maybe you can have a look here? (https://github.com/bitcoin/bitcoin/pull/30186#issuecomment-2135744973)
fanquake closed a pull request: "releases: use LLVM 18 for macOS"
(https://github.com/bitcoin/bitcoin/pull/30022)