Bitcoin Core Github
43 subscribers
122K links
Download Telegram
💬 hebasto commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541630314)
```suggestion
You might want to override native tool compilers when your default build compiler (set in `./depends/builders/*.mk`) is not available. For example, when using a Linux host without gcc/g++.
```
💬 hebasto commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541632430)
```suggestion
Example using Clang for native build tools on Linux:
```
🤔 l0rinc reviewed a pull request: "ci: Make the max number of commits tested explicit"
(https://github.com/bitcoin/bitcoin/pull/33909#pullrequestreview-3482229939)
I didn't realize this, so concept ACK.

I wouldn't include the value of a variable in the variable's name, though, and I think the job's name should likely also be updated - left a suggestion
💬 l0rinc commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2541635179)
6 is an implementation detail, the high-level point is to signal that not every commit is checked, but that the last few commits in the current PR are.
Also, the job's name should also likely be adjusted, maybe `ci-test-each-commit-exec.py` as well:

```suggestion
test-latest-commits:
name: 'test latest commits'
```
💬 fanquake commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3552230525)
Can we just update Qt?
⚠️ johnsonella2208-oss opened an issue: "Mighty Hacker Recovery Hope for Support Available for Your Ethereum/Crypto/Asset Recovery Case/ Private Wallet Key"
(https://github.com/bitcoin/bitcoin/issues/33910)
### Motivation

**Mighty Hacker Recovery Hope for Support Available for Your Ethereum/Crypto/Asset Recovery Case/ Private Wallet Key**

### Possible solution

**Mighty Hacker Recovery Hope for Support Available for Your Ethereum/Crypto/Asset Recovery Case/ Private Wallet Key**

### Useful Skills

* Compiling Bitcoin Core from source
* Running the C++ unit tests and the Python functional tests
* ...


### Guidance for new contributors

Want to work on this issue?

For guidance on contributing, pl
...
💬 hebasto commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3552247066)
> Can we just update Qt?

Updating Qt would require raising the minimum GCC version for Windows cross-compilation to [13.1](https://doc.qt.io/qt-6.8/supported-platforms.html#windows). Are we comfortable with that?
💬 willcl-ark commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541662001)
I agree, but going to leave that from this PR for now.
💬 willcl-ark commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541662319)
Taken in 75155c073d7
💬 willcl-ark commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541662701)
Taken in 75155c073d7
💬 willcl-ark commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541662993)
Taken in 75155c073d7
👍 hebasto approved a pull request: "Document compiler configuration for native depends packages"
(https://github.com/bitcoin/bitcoin/pull/33902#pullrequestreview-3482268948)
re-ACK 7dd714ae71fd18eda82ab4b43d4cecc047b87a2d.
💬 willcl-ark commented on pull request "Document compiler configuration for native depends packages":
(https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2541664757)
Reolving this based on wording in 75155c073d7.
💬 janb84 commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#issuecomment-3552252907)

> Also, the title seems the wrong place to list all the shortcomings of each CI task. For example, the win-cross does not build the gui tests, should it say 'no-gui-tests'? Or the arm test has `-Wno-error=maybe-uninitialized`, so should the title say 'no-error-uninit'? Some tasks do not have `CI_LIMIT_STACK_SIZE` set, some tasks don't run the previous releases, or the extended functional tests, some have the gui disabled, no task is using gcc-15, ...
>
> Maybe it makes sense to document thi
...
💬 fanquake commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3552262328)
Is this still relevant, given you've ACK'd #33770?
💬 kevkevinpal commented on pull request "ci: Make the max number of commits tested explicit":
(https://github.com/bitcoin/bitcoin/pull/33909#discussion_r2541695322)
is this the reason for the limitation? or are there other reasons?

if its just this couldn't we just get how many commits are in this branch and use it here?
💬 maflcko commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2541710794)
i don't think this heuristic makes sense, because it is already incomplete with the merge of this pull, and it is unclear how to even maintain it going forward. I don't think it is a good use of time to think about every single named argument, whether it can be a string, and whether the string may theoretically contain a `=`.

It would be better to just fully list all named args names in the client. Then, just check if the string starts with the named arg name. If yes, it is a named arg. If not,
...
💬 fanquake commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2541711810)
xcb & xkb libs can be dropped.
💬 fanquake commented on pull request "doc: Add `INSTALL.md` to Linux release tarballs":
(https://github.com/bitcoin/bitcoin/pull/33451#discussion_r2541716526)
I'm not sure we need to link to Qt docs here, given it's listing ~ 25 different things, of which only 2-3 are relevant to our binaries.
💬 theStack commented on pull request "test: add `-alertnotify` test for large work invalid chain warning":
(https://github.com/bitcoin/bitcoin/pull/33893#issuecomment-3552330703)
In addition to `feature_bip68_sequence.py`, it seems that the functional tests `feature_coinstatsindex.py`, `feature_pruning.py` and `mempool_reorg.py` hit this code-path as well (tested experimentally by putting an `assert(false)` into the branch to provoke a crash), most likely all of them in the course of an `invalidateblock` call, though I didn't check in detail. However, none of these use the `-alertnotify` option, so the function `AlertNotify` returns early in those cases.