Bitcoin Core Github
44 subscribers
120K links
Download Telegram
💬 HowHsu commented on pull request "index: Fix missing case in the comment in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#issuecomment-3306769093)
> ```c++
> CBlockIndex* NextInclRewind(const CBlockIndex* pindex) const
> {
> if (!Contains(Assert(pindex))) {
> pindex = Assert(FindFork(pindex));
> }
> return (*this)[pindex->nHeight + 1];
> }
> ```

Hi Luke,
If I'm getting you wrong, `NextInclRewind()` will be a method member of CChain, but from my perspective, this function seems
tailored to a very specific scenario. To keep the class more general and lightweight, it might be better
...
💬 ryanofsky commented on pull request "test: Avoid interface_ipc.py Duplicate ID errors":
(https://github.com/bitcoin/bitcoin/pull/33420#issuecomment-3306783651)
> I also haven't reproduced the issue, using the steps provided. i.e installing libmulitprocess system-wide in a new VM, and then building Core & running `interface_ipc.py`.

Yeah, an additional requirement for the bug to happen is that cap'nproto and libmultiprocess need to be installed in the same prefix. If you are using a system provided cap'n proto installed in `/usr` and a locally build libmultiprocess installed in `/usr/local` the bug won't happen. They both need to be installed in `/us
...
💬 HowHsu commented on pull request "checkqueue: implement a new scriptcheck worker pool with atomic variables":
(https://github.com/bitcoin/bitcoin/pull/32791#issuecomment-3306849241)
> > hmmm...Ok, I realized that this may only be suitable for when all the preout Coin are in CCoinsViewCache, otherwise the `Add()` is not fast due to I/O there.
>
> With #31132 , this one makes sense again, need to test them together in all-cache-miss case

Tested this one and [](https://github.com/bitcoin/bitcoin/pull/31132) combined with about 100+ contiguous real blocks in mainnet from height 840000, by the method I mentioned in [](https://github.com/bitcoin/bitcoin/issues/33375#issueco
...
📝 fanquake opened a pull request: "ci: run native_fuzz_with_msan"
(https://github.com/bitcoin/bitcoin/pull/33425)
How fast does this run (minus the Clang build)?
⚠️ janb84 opened an issue: "Guix: build riscv64 master / v30rc1 fails"
(https://github.com/bitcoin/bitcoin/issues/33426)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

While building a guix-build of v30rc1 or main for `riscv64-linux-gnu` architecture fails with :

```console
[100%] Linking CXX executable ../../bin/test_bitcoin
riscv64-linux-gnu-ld: CMakeFiles test_bitcoin.dir/main.cpp.o: in function "boost::fpe::disable(unsigned int)':
bitcoin depends/riscv64-linux-gnu/include/boost/test/impl/execution_monitor.ipp:1538: warning: fedisableexcept is not i
...
👋 fanquake's pull request is ready for review: "ci: run native_fuzz_with_msan"
(https://github.com/bitcoin/bitcoin/pull/33425)
📝 fanquake converted_to_draft a pull request: "ci: run native_fuzz_with_msan"
(https://github.com/bitcoin/bitcoin/pull/33425)
How fast does this run (minus the Clang build)?
💬 janb84 commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3306961384)
Linking to https://github.com/bitcoin/bitcoin/issues/33426 (semi related to this testing)
Guix build for riscv64 fails on master & 30.0rc1
💬 fanquake commented on issue "Guix: build riscv64 master / v30rc1 fails":
(https://github.com/bitcoin/bitcoin/issues/33426#issuecomment-3306969096)
Can you provide more info. These warnings have existed for years (i.e are in the 29.x branch that builds for you), and build warnings shouldn't cause guix build failures.
💬 janb84 commented on issue "Guix: build riscv64 master / v30rc1 fails":
(https://github.com/bitcoin/bitcoin/issues/33426#issuecomment-3307000340)
going to hunt down some more information.

The difference between 29.x and master / v30.0.rc1 on line 146 of the make file is IPC:

line 146:
```cmake
if(ENABLE_IPC AND WITH_EXTERNAL_LIBMULTIPROCESS)
find_package(Libmultiprocess REQUIRED COMPONENTS Lib)
find_package(LibmultiprocessNative REQUIRED COMPONENTS Bin
NAMES Libmultiprocess
)
endif()
```
🤔 hodlinator reviewed a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3238340125)
Reviewed f6951cb74bba60fedb2e0c041038561ba2583594
💬 hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2358151635)
nit in 788e45a5b210224e476543cbd31dffc7ae39b13e: Some sympathy for unwrapping the initial sentence since with the (and/an) typo fix it's just at 80 chars, but 4 space indent seems less arbitrary than 1 space, and not worth touching these lines for.
💬 hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2358241717)
```suggestion
p2p3 = self.nodes[3].add_p2p_connection(BaseNode())
```
💬 hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2358167560)
nit in 9ecaadc94a5a4a61263aa17ddaba23ee5d155042: Think there is some value in benefit from nodes starting up in parallel with the python code running. Would prefer starting them all here or even sooner, before the loop generating 2100 blocks above.

Sorry if my suggestion in https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2351892330 precipitated this, I was just trying to reduce complexity in the example.
💬 hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2358189439)
In b577becd7167593ba55f1f084cfded59f4c11e47: Touching this line to reduce it by 1 char seems uncalled for now that you are keeping the name.
💬 hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2358761689)
nit: I'd prefer avoiding having this dual state. Here are 2 variations which do that:

Preserving behavior:

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 7e82c3bbad..008bfe4d71 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2423,8 +2423,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
return true;
}

- auto fScriptChecks{true};
- std::string script_check_reason;
+ const char* script_che
...
💬 hodlinator commented on pull request "log: always print initial signature verification state":
(https://github.com/bitcoin/bitcoin/pull/33336#discussion_r2358840909)
Yes, I would prefer having a separate reason message for when we have passed beyond the assumevalid height since it's so fundamental to assumevalid, and so different from being at a height below assumevalid on a forked chain.

Agree it probably belongs in a separate commit, but still think it would be good to have in this PR.
Your first commit that currently introduces `"block not part of assumevalid chain"` should probably state something more neutral like `"block is not an ancestor of the a
...
💬 hebasto commented on issue "Guix: build riscv64 master / v30rc1 fails":
(https://github.com/bitcoin/bitcoin/issues/33426#issuecomment-3307151918)
@janb84

Could you please check your build log for the actual error message, which may appear many lines earlier?
💬 hebasto commented on issue "Guix: build riscv64 master / v30rc1 fails":
(https://github.com/bitcoin/bitcoin/issues/33426#issuecomment-3307217163)
> Added console log, and build logs
>
> [console.log](https://github.com/user-attachments/files/22406285/console.log)

Does it include stderr output as well?