Bitcoin Core Github
44 subscribers
121K links
Download Telegram
📝 hebasto opened a pull request: "cmake: Set top-level target output locations"
(https://github.com/bitcoin/bitcoin/pull/31161)
This PR sets the target output locations to the `bin` and `lib` subdirectories within the build tree, creating a directory structure that mirrors that of the installed targets.

This approach is widely adopted by the large projects, such as [LLVM](https://github.com/llvm/llvm-project/blob/e146c1867e8decfd423034f63a3a863733e03f04/lldb/cmake/modules/LLDBStandalone.cmake#L128-L130):
```cmake
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_
...
⚠️ carnhofdaki opened an issue: "Stop at header"
(https://github.com/bitcoin/bitcoin/issues/31162)
### Please describe the feature you'd like to see added.

While preparing the little ter.gz file for https://github.com/fjahr/test_chain_init/issues/1 I stumbled upon the fact that one is unable to download just a couple of blocks in the beginning without synchronizing the full header chain.

I would be happy if the `stopatheight` option was respected also header-wise. This is a feature request.

Using 28.0 x86_64 Linux release binaries from bitcoincore.org

```
$ bitcoind -help -help-deb
...
👍 tdb3 approved a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2397262411)
re ACK f383db76ec3aaa9391509c1d9cca763d11b6fe00

Changes include updating `scriptpubkey_hex` result type to `STR_HEX`, cleaner `Coin` usage, and adding a test for unconfirmed tx.

<details>
<summary>git range-diff d79ea809d28197b1b4e3748aa1715272b53601d0..1365ee8e9c7a20aa63bcddb1a6d5843c05ff9330 25dacae9c7feb31308271e2fd5a127c1fc230c2f..f383db76ec3aaa9391509c1d9cca763d11b6fe00</summary>

```diff
1: 1365ee8e9c7 ! 1: f383db76ec3 rpc: add getdescriptoractivity
@@ src/rpc/blockchain.c
...
💬 TheCharlatan commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2439601361)
Concept ACK
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817860890)
I haven't looked lately but these harnesses should trigger package RBF behavior. I added the package mempool results checking to check the related invariants.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817860896)
updated
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817860901)
yeah that's confusing, reworded
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817860919)
correct, updated comment
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817860922)
done
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1817860928)
out of date logging, updated
💬 jadijadi commented on issue "か":
(https://github.com/bitcoin/bitcoin/issues/29609#issuecomment-2439602668)
Is this spam / scam? I believe this can be closed.
fanquake closed an issue: "か"
(https://github.com/bitcoin/bitcoin/issues/29609)
:lock: fanquake locked an issue: "か"
(https://github.com/bitcoin/bitcoin/issues/29609)
🤔 hodlinator reviewed a pull request: "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests"
(https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2397249971)
Concept ACK ad917fb105b68fb369455ec7c42292a7673db1ad

- Extra tests/coverage, does not modify files containing the functionality being tested.
- Does remove `base58_random_encode_decode` unit test, but everything seems to be carried over to the modified fuzz test (except for testing for a variable count of insufficient result buffer sizes).
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817848501)
Should keep using `TrimStringView` here to avoid allocating extra `std::string` instances. Fuzzing code is re-run at high frequency so optimization (without compromising code coverage) is important.
```suggestion
assert(encoded_string == TrimStringView(random_string));
```
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817851827)
Why add the space?

Could also verify error is returned correctly.
```suggestion
assert(DecodeBase64PSBT(psbt, random_string, error) == error.empty());
```
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817849205)
I understand the change to use `random_string` as something that might either be encoded already, or as something to be encoded in the later half of these functions, so the rename makes sense. Would avoid stripping `const` for these slightly longer lived variables, just for peace of mind.
```suggestion
const std::string random_string{buffer.begin(), buffer.end()};
const std::vector<unsigned char> random_data{ToByteVector(random_string)};
```
---
Another aspect is that `buffer` has
...
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817857203)
(My instinct would be to go with the following to avoid rocking the boat when it comes to seed corpus naming etc, but I'm too new to this aspect of fuzzing to carry much weight in this aspect).
```C++
void base58_encode_decode(FuzzBufferType buffer);
void base58check_encode_decode(FuzzBufferType buffer);
void base32_encode_decode(FuzzBufferType buffer);
void base64_encode_decode(FuzzBufferType buffer);
void psbt_base64_decode(FuzzBufferType buffer);

FUZZ_TARGET(base_encode_decode)
{

...
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817854496)
Good that you removed the normalizing of the case for Base58 roundtripping as case is significant there (unlike how we treat base32). :+1:
💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1817861317)
Could test slightly more aggressively in line with the removed test:
https://github.com/bitcoin/bitcoin/blob/a5b4e42a575b4680a9eedb6ab2275d2a723cc20d/src/test/base58_tests.cpp#L93-L94

```suggestion
assert(encoded_string.empty() || !DecodeBase58(encoded_string, decoded, fuzzed_data_provider.ConsumeIntegralInRange(0, decoded.size() - 1)));
```