Bitcoin Core Github
42 subscribers
126K links
Download Telegram
📝 eval-exec reopened a pull request: "torcontrol: Define tor reply code as const to improve our maintainability"
(https://github.com/bitcoin/bitcoin/pull/32166)
This PR want to replace tor repy code with const to improve out maintainability.

Friendly invite @fanquake @laanwj @luke-jr to review this PR.
eval-exec closed a pull request: "torcontrol: Add comment explaining Proxy credential randomization for Tor privacy"
(https://github.com/bitcoin/bitcoin/pull/31973)
💬 eval-exec commented on pull request "torcontrol: Add comment explaining Proxy credential randomization for Tor privacy":
(https://github.com/bitcoin/bitcoin/pull/31973#issuecomment-2764512464)
Has cherry-pick this commit into https://github.com/bitcoin/bitcoin/pull/32166
⚠️ maflcko opened an issue: "oss-fuzz build fails"
(https://github.com/bitcoin/bitcoin/issues/32167)
https://oss-fuzz-build-logs.storage.googleapis.com/index.html#bitcoin-core

Passing: https://oss-fuzz-build-logs.storage.googleapis.com/log-412269f7-edf7-41df-bf45-c7c19e4a6518.txt

(commit f1d129d96340503ec5f6b347c2fdf6a6901b1f7e)

Failing: https://oss-fuzz-build-logs.storage.googleapis.com/log-55825031-7dd1-470e-a768-1dd6d1cc1d8a.txt

(commit 0a1e36effaa5aa27bc5f2c4955c1fa2eb2f0c8fa)

output:

```
Step #30 - "compile-libfuzzer-address-i386": copying packages: boost libevent sqlite
Step #30 - "
...
💬 maflcko commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r2020141507)
No strong opinion, but I think we shouldn't spend too much time on the error message explaining the context because the traceback could contain too little context, or on the traceback containing too much (redundant) context by default. A full log of a test failure could be thousands of lines, so one additional line, even if could be redundant in a strict sense should be fine. Also, looking at the historic failures (in Python) at https://github.com/bitcoin/bitcoin/issues?q=is%3Aissue%20%20label%3
...
💬 maflcko commented on pull request "refactor: Enforce readability-avoid-const-params-in-decls":
(https://github.com/bitcoin/bitcoin/pull/31650#discussion_r2020144640)
The goal of this pull is to avoid `const` params in declarations, not to avoid `const` params in definitions.

I can understand the confusion, so I am happy to do any of the following:

* Close this pull
* Just take the first commit and drop the second one
* Add a commit to remove const in definitions as well, though, this at this point we may enter the territory where it isn't worth it to spend review time on this refactor
📝 katesalazar opened a pull request: "doc/descriptors.md: Update next halvening heights"
(https://github.com/bitcoin/bitcoin/pull/32168)
You are welcome to remix these changes in your own way.
💬 0xNegnin commented on issue "v29.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/32052#issuecomment-2764547856)
I also have upgraded to v29.0rc2 without any issue using the following stack:

- bitcoind
- electrs
- nginx (as reverse proxy for electrs)
- debian12
💬 fjahr commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#discussion_r2020166642)
That would be nice but I'm not sure how to do it. The one candidate index we have for that is `txindex` but we wholly disable that in the context of pruning because what it is used for is incompatible with pruning.
💬 fjahr commented on pull request "index: Check all necessary block data is available before starting to sync":
(https://github.com/bitcoin/bitcoin/pull/29770#issuecomment-2764580300)
I am making one more effort to pick this up see if it can get the necessary support. Upon re-reading our conversation @furszy I have removed the `chain.h` dependency from the indices so that it's a bit closer to what #24230 is planning to do. I looked at it as well to see if there are commits available to pick to add a options object but that didn't seem to be the case and #24230 is also still in draft mode so unsure how valuable it would be if I pick some small piece of code out of it. The code
...
💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r2020168731)
This could also be useful to avoid the needless DB_HEAD_BLOCKS erase and rewrite for empty batches.
📝 l0rinc converted_to_draft a pull request: "[IBD] flush UTXOs in bigger batches"
(https://github.com/bitcoin/bitcoin/pull/31645)
This change is part of [[IBD] - Tracking PR for speeding up Initial Block Download](https://github.com/bitcoin/bitcoin/pull/32043)

### Summary

The final UTXO set is [written to disk in batches](https://github.com/bitcoin/bitcoin/blob/master/src/txdb.cpp#L130-L133) to avoid a gigantic spike at flush time.
There is already a [-dbbatchsize](https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L490) config option to change this value, this PR adjusts the default only.
By increasing th
...
💬 bigspider commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#issuecomment-2764601895)
I added a workaround to disable the two failing tests from the `script_assets_test`; those tests do indeed assume the `OP_SUCCESS` behaviour of the `0xBB` opcode.
💬 l0rinc commented on pull request "[IBD] flush UTXOs in bigger batches":
(https://github.com/bitcoin/bitcoin/pull/31645#issuecomment-2764621216)
I've experimented a bit with changing the fixed write batch size to a dynamic value that scales with dbcache size, basically something like:
```C++
static int64_t GetDefaultDbBatchSize(int64_t dbcache_bytes) {
return std::min<int64_t>(
MAX_DB_CACHE_BATCH, // 512 MiB upper limit
std::max<int64_t>(
MIN_DB_CACHE, // 4 MiB lower limit
dbcache_bytes * DEFAULT_KERNEL_CACHE_BATCH_SIZE / DEFAULT_KERNEL_CACHE
)
);
}
```

Corresponding
...
🤔 fjahr reviewed a pull request: "wallet, migration: Fix empty wallet crash"
(https://github.com/bitcoin/bitcoin/pull/32149#pullrequestreview-2727937771)
Concept ACK
💬 fjahr commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020190503)
```suggestion
//! Returns true if there are any DBKeys::LEGACY_TYPES record in the wallet db
```
💬 fjahr commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020189978)
Seems like this can be simplified (with maybe an additional comment that an empty wallet means automatic success)

```suggestion
{
// Keep the batch alive only for this call
const auto& db_batch = local_wallet->GetDatabase().MakeBatch();
success = !HasLegacyRecords(local_wallet.get(), *db_batch);
}

// Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
```
💬 fjahr commented on pull request "wallet, migration: Fix empty wallet crash":
(https://github.com/bitcoin/bitcoin/pull/32149#discussion_r2020190409)
nit: I find this comment a bit redundant, especially in the tests. If you rename `db_batch` to `temp_batch` IMO it could be removed.
💬 EthanHeilman commented on pull request "tests: improves tapscript unit tests":
(https://github.com/bitcoin/bitcoin/pull/31640#issuecomment-2764707199)
> could you squash commits?

@instagibbs
Squashed
💬 l0rinc commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2764716714)
> It seems odd that using exact block sizes (this pull initially) passes, whereas using a 1<<20 approximation fails due to OOM

Yes, I thought of the same, but the other PR had the same issue. I'm not familiar enough with the intricacies of `ASan + LSan + UBSan + integer, no depends, USDT` to have a good enough explanation - besides the instrumentation causing it to overflow.

> What is the memory usage of the CI before and after?

I have measured a valgrind/massif IBD until the be benchma
...