Bitcoin Core Github
44 subscribers
119K links
Download Telegram
πŸ€” tdb3 reviewed a pull request: "Policy: Report reason inputs are non standard"
(https://github.com/bitcoin/bitcoin/pull/29060#pullrequestreview-2406688998)
I'm a fan of the more descriptive errors.

However, would some of these more descriptive errors flow down to RPC responses? (e.g. `testmempoolaccept`).

https://github.com/bitcoin/bitcoin/pull/30212#issuecomment-2347371722
> Does bitcoind consider this to be a breaking change? The new error message may be "more accurate", but for clients that were matching on this error to figure out why a transaction was rejected, this breaks old behavior.
πŸ’¬ tdb3 commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1823630805)
Could also clarify that invalid TxValidationState returned describes the first nonstandard input encountered.

For example:
```diff
- * invalid TxValidationState which states why an input is not standard
+ * invalid TxValidationState which states why the first invalid input is not standard
```
πŸ’¬ tdb3 commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r1823661556)
Instantiating the CScript here could allow re-use later (in `txToNonStd4.vin[0].scriptSig = CScript(op_return, op_return + sizeof(op_return));`)
πŸ’¬ sipa commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2448864279)
This means that no `Assume` can be compiled out in production builds anymore, as they all involve a `g_fuzzing` check?
πŸ’¬ sipa commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2448867990)
@m3dwards Thanks for digging! I believe that explains it: since #31093, `Assume()` calls are no longer optimized out in production builds.
πŸ’¬ Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2448891405)
I'm still confused why Ninja is needed and why nothing complains when it's missing.

> Ninja is required to build the qt package in depends. It is mentioned in depends/README.md.

It only mentions it in the context of an Ubuntu & Debian install.

I did a depends build for af05dd9a12b89224dc7ad229698eeceb3e560ed4 again on Intel macOS 15.0.1 with Xcode 16 and no Ninja. It seems to work fine, at first glance.

I didn't try without Xcode since my laptop seems to be in a weird state: https:
...
πŸ’¬ davidgumberg commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2448905871)
> @m3dwards Thanks for digging! I believe that explains it: since #31093, `Assume()` calls are no longer optimized out in production builds.

I've reproduced this performance issue locally, and when you remove `g_fuzzing` from `inline_assertion_check` the regression goes away:

`./build/src/bench/bench_bitcoin -filter=LinearizeOptimallyExample11 -min-time=30000`

|branch | ns/op | op/s | err% | ins/op | cyc/op | IPC | bra/op
...
πŸ’¬ Khanwafa232426 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/6e21dedbf2b3029c729108f225469b321a1b3d39#r148579531)
TTBp1cVNxDHUxetKwXof2cLfeRFWMqejhV
πŸ’¬ Khanwafa232426 commented on something "":
(https://github.com/bitcoin/bitcoin/commit/6e21dedbf2b3029c729108f225469b321a1b3d39#r148579539)
TTBp1cVNxDHUxetKwXof2cLfeRFWMqejhVTTBp1cVNxDHUxetKwXof2cLfeRFWMqejhVhttps://www.binance.com/fan-token/BWS/FCShakhtarFanverse?ref=CG5V70EE&registerChannel=fcshakhtar&utm_source=fan-token&utm_campaign=fcshakhtar-fanverse-invite
πŸ’¬ laanwj commented on issue "Gracefully handle dropped UPnP support ":
(https://github.com/bitcoin-core/gui/issues/843#issuecomment-2449234882)
Even ignoring the spurious setting would be better in the GUI case. No need to rub this in the users face. i didn't expect this consequence when i suggested adding a message πŸ˜…

Though it's still better than creashing with generic "-upnp: Unknown setting". i guess...

But yes this needs to be informational level at most, it should just continue.

Even worse: i guess it even happens if the settings json specifies upnp=0 to explicitly disable it?
πŸ“ maflcko converted_to_draft a pull request: "ci: Place datadirs for tests under tmpfs "
(https://github.com/bitcoin/bitcoin/pull/31182)
This should speed up the tests a bit (see comment below for results).
πŸ’¬ maflcko commented on pull request "ci: Place datadirs for tests under tmpfs ":
(https://github.com/bitcoin/bitcoin/pull/31182#issuecomment-2449255282)
> > (see comment below for results).
>
> What is this referring to?

I was still running the commit (and master) twice each for all CI tasks, but I couldn't find any difference at all.

I checked that the folder is correctly picked up and used by the tests, so I guess modern storage is fast enough to not make a difference? Alternatively, there is an issue I was missing.

It would be good if someone else double checked the code and whether there is a performance difference at all.

For
...
πŸ’¬ maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2449259598)
> > ramdisk
>
> Benchmarking this is a bit more involved and I think requires changes to the CI scripts, so that the ramdisk is (1) mounted into the CI container and (2) picked up as the temp dir. I can take a look in the future, unless someone beats me to it.

Done in https://github.com/bitcoin/bitcoin/pull/31182, but I couldn't find any difference at all so far, which is a bit surprising.
πŸ’¬ l0rinc commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449260674)
Could we obviate in the [DrahtBot's response](https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2413637472) that the link doesn't just provide a `test coverage report` - since the regression was already visible there: https://corecheck.dev/bitcoin/bitcoin/pulls/31093
πŸ’¬ naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1824060041)
I was looking for something like "As a general rule, we keep the transaction that appeared first".

Currently, the `AddToSet` documentation seems outdatedβ€”it basically claims that the only way to fail is to reach a set limit.

How about the following replacement:

```
/**
* Step 1. Add a to-be-announced transaction to the local reconciliation set of the target peer.
* Returns false if the set is at capacity, or if the set contains a colliding transaction.
* Returns t
...