💬 real-or-random commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1466598461)
My take on `SECP_CFLAGS` is that it's an internal of the libsecp256k1 build system, and users are not supposed to set it (and are not guaranteed that it does anything meaningful). The proper user variable is just `CFLAGS`.
You could either export `CFLAGS="-g $SANITIZER_CFLAGS"` temporarily, or perhaps add a CFLAGS arg to `ac_configure_args`.
We could also expose the `SECP_CFLAGS` "officially" in the lib. Then it should probably be renamed to `SECP256K1_CFLAGS`. But I think it's a bit arbi
...
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1466598461)
My take on `SECP_CFLAGS` is that it's an internal of the libsecp256k1 build system, and users are not supposed to set it (and are not guaranteed that it does anything meaningful). The proper user variable is just `CFLAGS`.
You could either export `CFLAGS="-g $SANITIZER_CFLAGS"` temporarily, or perhaps add a CFLAGS arg to `ac_configure_args`.
We could also expose the `SECP_CFLAGS` "officially" in the lib. Then it should probably be renamed to `SECP256K1_CFLAGS`. But I think it's a bit arbi
...
💬 real-or-random commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1466601203)
Serious question: What is this line supposed to print, given that there are also `ZMQ_CFLAGS`, `BDB_CFLAGS` and `EVENT_CFLAGS`? One could say that the libsecp256k1 configure output already prints its CFLAGS, so there's no need to cover that here, but I don't know because I don't know what the purpose of this line is. Where else do we need CFLAGS?
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1466601203)
Serious question: What is this line supposed to print, given that there are also `ZMQ_CFLAGS`, `BDB_CFLAGS` and `EVENT_CFLAGS`? One could say that the libsecp256k1 configure output already prints its CFLAGS, so there's no need to cover that here, but I don't know because I don't know what the purpose of this line is. Where else do we need CFLAGS?
💬 fanquake commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1466607662)
I was thinking about the best way to handle this, and my current take was that given this is only when we are using sanitizers, and we are a somewhat non-standard/more-tightly-coupled users of secp in any case, I wasn't too worried. Happy for any other approach that achieves the same thing, but also don't want secp to expose additional (non-standard) flags just for us.
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1466607662)
I was thinking about the best way to handle this, and my current take was that given this is only when we are using sanitizers, and we are a somewhat non-standard/more-tightly-coupled users of secp in any case, I wasn't too worried. Happy for any other approach that achieves the same thing, but also don't want secp to expose additional (non-standard) flags just for us.
⚠️ sdaftuar opened an issue: "Cluster mempool, CPFP carveout, and V3 transaction policy"
(https://github.com/bitcoin/bitcoin/issues/29319)
Opening an issue for high-level discussion, as the PR that implements this has gotten difficult to follow.
### Cluster mempool
Work is underway to [redesign the mempool](https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393/1) with different topology constraints on the transaction graph than exist today. I originally described this proposal in a github issue (#27677), and have shared a draft implementation (#28676). In brief, with a new mempool design we could simu
...
(https://github.com/bitcoin/bitcoin/issues/29319)
Opening an issue for high-level discussion, as the PR that implements this has gotten difficult to follow.
### Cluster mempool
Work is underway to [redesign the mempool](https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393/1) with different topology constraints on the transaction graph than exist today. I originally described this proposal in a github issue (#27677), and have shared a draft implementation (#28676). In brief, with a new mempool design we could simu
...
💬 hebasto commented on issue "Unable to open wallet UI with ubuntu 23.10":
(https://github.com/bitcoin/bitcoin/issues/29311#issuecomment-1910533393)
On Ubuntu 23.10, I've installed the Bitcoin Core 26.0 from the "latest/stable" Snap channel.
> The terminal output of the command line
> `Fontconfig warning: FcPattern object weight does not accept value [0 205) [1] 824648 segmentation fault (core dumped) env /snap/bin/bitcoin-core.qt %u`
Cannot reproduce it.
> Hello Thank you this command line works [keshavbhatt/olivia#95 (comment)](https://github.com/keshavbhatt/olivia/issues/95#issuecomment-774747492)
Indeed.
> Close or move
...
(https://github.com/bitcoin/bitcoin/issues/29311#issuecomment-1910533393)
On Ubuntu 23.10, I've installed the Bitcoin Core 26.0 from the "latest/stable" Snap channel.
> The terminal output of the command line
> `Fontconfig warning: FcPattern object weight does not accept value [0 205) [1] 824648 segmentation fault (core dumped) env /snap/bin/bitcoin-core.qt %u`
Cannot reproduce it.
> Hello Thank you this command line works [keshavbhatt/olivia#95 (comment)](https://github.com/keshavbhatt/olivia/issues/95#issuecomment-774747492)
Indeed.
> Close or move
...
💬 sdaftuar commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1910540024)
cc: @TheBlueMatt @rustyrussell @Roasbeef @t-bast
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1910540024)
cc: @TheBlueMatt @rustyrussell @Roasbeef @t-bast
💬 real-or-random commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1466616567)
> I wasn't too worried.
Yep, I agree, any approach is ok in the end. My feeling is just that setting `SECP_CFLAGS` is not the cleanest choice and more likely than my other suggestions to cause breaks in the future.
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1466616567)
> I wasn't too worried.
Yep, I agree, any approach is ok in the end. My feeling is just that setting `SECP_CFLAGS` is not the cleanest choice and more likely than my other suggestions to cause breaks in the future.
💬 Retropex commented on pull request "Add a `-permitbarepubkey` option":
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1910542474)
tACK https://github.com/vostrnad/bitcoin/commit/8c1114aa61c46e58efb44368b5428d3ccb65f9d0
Steps to test:
1. Build [vostrnad branch](https://github.com/vostrnad/bitcoin/tree/permitbarepubkey)
1. Start a regtest with `-chain=regtest`
1. Create a wallet with `bitcoin-cli createwallet`
1. Get address with `bitcoin-cli getnewaddress`
1. Generate blocks to get bitcoins `generatetoaddress 500 <address obtained previously>`
1. Create a P2PK tx, this [repository](https://github.com/dianerey/bec
...
(https://github.com/bitcoin/bitcoin/pull/29309#issuecomment-1910542474)
tACK https://github.com/vostrnad/bitcoin/commit/8c1114aa61c46e58efb44368b5428d3ccb65f9d0
Steps to test:
1. Build [vostrnad branch](https://github.com/vostrnad/bitcoin/tree/permitbarepubkey)
1. Start a regtest with `-chain=regtest`
1. Create a wallet with `bitcoin-cli createwallet`
1. Get address with `bitcoin-cli getnewaddress`
1. Generate blocks to get bitcoins `generatetoaddress 500 <address obtained previously>`
1. Create a P2PK tx, this [repository](https://github.com/dianerey/bec
...
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466618909)
Mmm, I tried something similar in de26cbc79230f64d7747b8b88f4dee71ebd37e1b by adding a `DOCKER_RUN` variable and setting it to `podman run --replace`. But this fails with `Error: short-name resolution enforced but cannot prompt without a TTY`. Are you passing any other arguments in your alias?
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466618909)
Mmm, I tried something similar in de26cbc79230f64d7747b8b88f4dee71ebd37e1b by adding a `DOCKER_RUN` variable and setting it to `podman run --replace`. But this fails with `Error: short-name resolution enforced but cannot prompt without a TTY`. Are you passing any other arguments in your alias?
🤔 pablomartin4btc reviewed a pull request: "debugwindow: update session ID tooltip"
(https://github.com/bitcoin-core/gui/pull/788#pullrequestreview-1844140068)
Concept ACK
nit: @MarnixCroes, maybe you can add the screenshot for transport v1 and session ID is empty/ not shown, if possible, even this PR doesn't affect that logic.
(https://github.com/bitcoin-core/gui/pull/788#pullrequestreview-1844140068)
Concept ACK
nit: @MarnixCroes, maybe you can add the screenshot for transport v1 and session ID is empty/ not shown, if possible, even this PR doesn't affect that logic.
💬 fanquake commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466623922)
> Are you passing any other arguments in your alias?
The alias came from:
```bash
apt install podman-docker
# podman-docker is already the newest version (4.8.3+ds1-2).
docker --version
# podman version 4.8.3
podman --version
# podman version 4.8.3
```
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466623922)
> Are you passing any other arguments in your alias?
The alias came from:
```bash
apt install podman-docker
# podman-docker is already the newest version (4.8.3+ds1-2).
docker --version
# podman version 4.8.3
podman --version
# podman version 4.8.3
```
💬 fanquake commented on pull request "ci: Use LLVM 18.x & DEBUG=1 in depends for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27495#discussion_r1466634562)
```bash
CMake Error at /msan/llvm-project/libcxxabi/CMakeLists.txt:51 (message):
LIBCXXABI_USE_LLVM_UNWINDER is set to ON, but libunwind is not specified in
LLVM_ENABLE_RUNTIMES.
-- Configuring incomplete, errors occurred!
```
(https://github.com/bitcoin/bitcoin/pull/27495#discussion_r1466634562)
```bash
CMake Error at /msan/llvm-project/libcxxabi/CMakeLists.txt:51 (message):
LIBCXXABI_USE_LLVM_UNWINDER is set to ON, but libunwind is not specified in
LLVM_ENABLE_RUNTIMES.
-- Configuring incomplete, errors occurred!
```
💬 fanquake commented on pull request "ci: Use LLVM 18.x & DEBUG=1 in depends for MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27495#discussion_r1466637149)
Added libunwind the the libc++ build as well
(https://github.com/bitcoin/bitcoin/pull/27495#discussion_r1466637149)
Added libunwind the the libc++ build as well
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466637332)
podman-docker is not documented in the manual installation... https://podman.io/docs/installation#building-from-source
Let's see how I can install this without totally messing up my OS :-)
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466637332)
podman-docker is not documented in the manual installation... https://podman.io/docs/installation#building-from-source
Let's see how I can install this without totally messing up my OS :-)
💬 achow101 commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1910567093)
> seems like something I would never expect to happen unless someone modified the code or manually corrupted the data on disk. So I'm wondering did we accidentally cause a breakage like that during development of these PRs?
It shouldn't ever happen.
In general, we don't have very robust error handling for when the database fails to read or write something. I think this should actually be more like an assert, but one that doesn't kill the whole software, maybe just unloads the wallet?
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1910567093)
> seems like something I would never expect to happen unless someone modified the code or manually corrupted the data on disk. So I'm wondering did we accidentally cause a breakage like that during development of these PRs?
It shouldn't ever happen.
In general, we don't have very robust error handling for when the database fails to read or write something. I think this should actually be more like an assert, but one that doesn't kill the whole software, maybe just unloads the wallet?
💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466643431)
You can try:
```sh
mkdir -p ~/bin/ && echo 'IyEvdXNyL2Jpbi9lbnYgYmFzaAoKZWNob2VycigpIHsgZWNobyAiJEAiIDE+JjI7IH0KCmVjaG9lcnIgIiAoOjo6ZG9ja2VyLT4pcG9kbWFuICR7QH0iCgpwb2RtYW4gIiR7QH0iCg==' | base64 --decode > ~/bin/docker && chmod +x ~/bin/docker
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466643431)
You can try:
```sh
mkdir -p ~/bin/ && echo 'IyEvdXNyL2Jpbi9lbnYgYmFzaAoKZWNob2VycigpIHsgZWNobyAiJEAiIDE+JjI7IH0KCmVjaG9lcnIgIiAoOjo6ZG9ja2VyLT4pcG9kbWFuICR7QH0iCgpwb2RtYW4gIiR7QH0iCg==' | base64 --decode > ~/bin/docker && chmod +x ~/bin/docker
💬 glozow commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466648992)
Do you mean all of the sibling eviction logic? That doesn't work because we'd only enforce that the tx pays enough to evict siblings OR conflicting transactions; it needs to pay for both.
If you mean the logic adding stuff to `m_conflicts`, `m_iters_conflicting`, etc., it's because I don't like in-out parameters.
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466648992)
Do you mean all of the sibling eviction logic? That doesn't work because we'd only enforce that the tx pays enough to evict siblings OR conflicting transactions; it needs to pay for both.
If you mean the logic adding stuff to `m_conflicts`, `m_iters_conflicting`, etc., it's because I don't like in-out parameters.
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466651006)
That base64 decodes to:
```sh
#!/usr/bin/env bash
echoerr() { echo "$@" 1>&2; }
echoerr " (:::docker->)podman ${@}"
podman "${@}"
```
That doesn't seem any different from my `DOCKER_RUN="podman run --replace"` alias.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466651006)
That base64 decodes to:
```sh
#!/usr/bin/env bash
echoerr() { echo "$@" 1>&2; }
echoerr " (:::docker->)podman ${@}"
podman "${@}"
```
That doesn't seem any different from my `DOCKER_RUN="podman run --replace"` alias.
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466652244)
Or I guess it's the `echoerr` doing some magic
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466652244)
Or I guess it's the `echoerr` doing some magic
💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466657588)
the later, but at least you gave a reason why not
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466657588)
the later, but at least you gave a reason why not