Bitcoin Core Github
43 subscribers
122K links
Download Telegram
⚠️ rkrux opened an issue: "Add `satToBtc()` and conversely `btcToSat()` util functions in functional tests"
(https://github.com/bitcoin/bitcoin/issues/31345)
### Motivation

In [functional tests](https://github.com/bitcoin/bitcoin/tree/master/test/functional), there are numerous instances of conversion code with patterns such as `/ COIN` and `* COIN` that are converting between units _satoshis_ to _BTC_.

Following details are as of the latest commit on master 2638fdb4f934be96b7c798dbac38ea5ab8a6374a.

### Patterns stats
```terminal
# satoshis to BTC conversion
➜ bitcoin git:(2638fdb4f9) ✗ git grep -n "/ COIN" -- '*.py' | wc -l
22


...
💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1853812830)
Quite a late reply here but I don't think I will get to this anytime soon, created an issue here for anyone else to pick up: https://github.com/bitcoin/bitcoin/issues/31345
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853756499)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853379963

The idea is not to add an ignored test, but to add real test coverage. For example if there is a buggy `add()` function that thinks `2 + 2 = 5`, the ideal way to fix it is to first add a test commit that asserts `add(2, 2) == 5` with a comment noting the add function is buggy. After this, a separate commit should fix the bug and update the test to assert `add(2, 2) == 4`. Advantages of this approach:

1. Clearly shows
...
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853811005)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853683566

> What's the reason for not wanting an `about` window regardless of the `-version` flag's value? Doesn't `gArgs.IsArgSet("-version")` make more sense here?

The idea is for -noversion and -version=0 to be the same as not specifying a -version argument. -version is just a binary option, it should choose between 2 behaviors, not 3
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853779968)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853551532

I think the idea here is just for -version to take precedence over -help if both are specified. I don't know if this is the best behavior, but I don't see a reason to change in a commit that is just trying to handle -version=0 and -noversion more sensibly.
💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853807905)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853700315

> given that checking the args is not for free (triggers locks and merges from multiple sources), maybe we could introduce a `IsArgEnabled` which gets rid of this confusing combination (e.g. where `-noconf=0` would be set and not negated, I think)

#31260 gets away from this error-prone repeated retrieval of settings by forcing the type of every setting to be declared up front, and only providing a single `Get` accesso
...
💬 l0rinc commented on issue "ci: how to run native arm job on Apple silicon?":
(https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493612110)
Not sure it helps, but I have tried reproducing it on my M4 Max, emulating an s390x platform (as I've done in https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2453108430) - while it's still very (very) slow:

<details>
<summary>docker setup</summary>

```bash
brew install podman pigz
softwareupdate --install-rosetta
podman machine init
podman machine start
docker run --platform linux/s390x -it ubuntu:22.04 /bin/bash
```

</details>

```bash
apt update && apt install -y
...
💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2493614105)
Well that's nice, looks like only the ASan + LSan + UBsan job is failing.
📝 Sjors opened a pull request: "Set notifications m_tip_block in LoadChainTip()"
(https://github.com/bitcoin/bitcoin/pull/31346)
Ensure KernelNotifications `m_tip_block` is set even if no new block arrives.

Suggested in https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2486457573
💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2493619203)
See #31346.
👍 rkrux approved a pull request: "contrib: skip missing binaries in gen-manpages"
(https://github.com/bitcoin/bitcoin/pull/30986#pullrequestreview-2454288885)
re-ACK 4bbd28baf33382231f4f1dab20681c05f9915af2

Minimal range diff.

<details>
<summary>git range-diff f080618...4bbd28b</summary>

```
1: 7060d64bf7 ! 1: 299e2220e9 gen-manpages: implement --skip-missing-binaries
@@ contrib/devtools/gen-manpages.py: BINARIES = [
+ "--skip-missing-binaries",
+ action="store_true",
+ default=False,
-+ help="skip generation for binaries that are not found",
++ help="skip generation for binaries that are no
...
💬 l0rinc commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1853828160)
nit: at this point it may make more sense to just remove the `[[nodiscard]]` from `blockTip` instead
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853836054)
I'm not against that, my concern is just that reproducing old behavior before the fix isn't very useful activity - it's a lot more useful to see that a test fails before the fix with the correct error (i.e TDD), but that's not really possible currently - unless we add something like https://github.com/spockframework/spock/blob/master/spock-core/src/main/java/spock/lang/PendingFeature.java#L13-L17 (where we invert the test, making sure it fails (that's when the test is green) and remove the annot
...
Sjors closed an issue: "ci: how to run native arm job on Apple silicon?"
(https://github.com/bitcoin/bitcoin/issues/31344)
💬 Sjors commented on issue "ci: how to run native arm job on Apple silicon?":
(https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493642543)
> It is not possible to run 32-bit arm binaries on a CPU that only supports 64-bit mode.

Ah, in that case it's probably pointless for a performance improvement.

> select qemu-arm in UTM. (Is there a reason why you haven't done this in the first place?)

My initial goal for that Ubuntu VM was to make Guix builds.

I could spin up a separate machine in 32 bit mode, but that seems like a world of pain on its own. Closing for now.
💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853838097)
what about `-noversion=0` :p

There has to be a better way, this is so confusing :/
💬 Sjors commented on issue "ci: how to run native arm job on Apple silicon?":
(https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493644965)
I guess my confusion came from the use of `arm64` as the label for cirrus.
💬 Sjors commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1853843127)
Maybe, or we could do something with the return value. I'm not familiar enough with init code and kernel notifications to have a strong opinion on this. cc @ryanofsky, @TheCharlatan
💬 maflcko commented on pull request "Set notifications m_tip_block in LoadChainTip()":
(https://github.com/bitcoin/bitcoin/pull/31346#issuecomment-2493675258)
This is incomplete. You'll have to update the docs as well: "If the tip was not connected on
* startup, this will wait.", and "which may
//! be true even long after startup, until shutdown.". Also, in init.cpp you can remove the if-guard?
💬 orangesurf commented on pull request "miner: Reorg Testnet4 minimum difficulty blocks":
(https://github.com/bitcoin/bitcoin/pull/31117#issuecomment-2493677462)
A miner (earning coinbase rewards to [tb1q2dsc...](https://mempool.space/testnet4/address/tb1q2dsc94zq40nwnz27w5rxljwllutnwjtlxk44fz) is currently overwhelmingly dominant on testnet4.

![image](https://github.com/user-attachments/assets/09db2517-29ae-403b-afbb-6ab7cbfdc926)

Since block [000000000000000167286a4bea3bca60d6c3ab2cbbef79537bdf07b6420a2d2d / 54780](https://mempool.space/testnet4/block/000000000000000167286a4bea3bca60d6c3ab2cbbef79537bdf07b6420a2d2d) this miner has mined every bl
...