💬 hebasto commented on pull request "[RFC] guix: remove xz from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2063547954)
Concept ACK on consolidating archive formats.
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2063547954)
Concept ACK on consolidating archive formats.
💬 maflcko commented on pull request "[RFC] guix: remove xz from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2063554137)
> Something a bit more concrete. We could consolidate all packages in depends to .tar.gz (which is also what we use for releases), and drop `bzip2` and eventually `xz`.
Again, I don't understand what the goal is. I doubt that removing xz as a dependency is doable. For example, gcc seems to be fetched as a xz archive (https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/gcc.scm#n803)? Even if it wasn't, I am sure there are other guix packages that depend on xz.
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2063554137)
> Something a bit more concrete. We could consolidate all packages in depends to .tar.gz (which is also what we use for releases), and drop `bzip2` and eventually `xz`.
Again, I don't understand what the goal is. I doubt that removing xz as a dependency is doable. For example, gcc seems to be fetched as a xz archive (https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/gcc.scm#n803)? Even if it wasn't, I am sure there are other guix packages that depend on xz.
💬 fanquake commented on pull request "[RFC] guix: remove xz from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2063559858)
> Again, I don't understand what the goal is.
The goal is to remove things we don't use. If we don't need bzip2, then I don't see a reason to keep it in our build release environment. Same reasoning for any other dep. Also, just because some thing may be used somewhere in the Guix bootstrap chain, isn't a reason to explicitly include it in our release env unnecessarily.
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2063559858)
> Again, I don't understand what the goal is.
The goal is to remove things we don't use. If we don't need bzip2, then I don't see a reason to keep it in our build release environment. Same reasoning for any other dep. Also, just because some thing may be used somewhere in the Guix bootstrap chain, isn't a reason to explicitly include it in our release env unnecessarily.
📝 willcl-ark opened a pull request: "Move bitcoin.conf to /share/examples dir in releases"
(https://github.com/bitcoin/bitcoin/pull/29903)
Motivated by #29139
Placing the example `bitcoin.conf` file in this directory makes it clearer that it will not actively be used by the binary in `bin/`, and must be moved elsewhere.
(https://github.com/bitcoin/bitcoin/pull/29903)
Motivated by #29139
Placing the example `bitcoin.conf` file in this directory makes it clearer that it will not actively be used by the binary in `bin/`, and must be moved elsewhere.
💬 laanwj commented on pull request "Move bitcoin.conf to /share/examples dir in releases":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063582138)
Good idea, code review ACK 9256693be16cd56183557cf0bef57979fd04c39c
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063582138)
Good idea, code review ACK 9256693be16cd56183557cf0bef57979fd04c39c
💬 fanquake commented on pull request "Move bitcoin.conf to /share/examples dir in releases":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063589430)
Note previous discussion / change in https://github.com/bitcoin/bitcoin/pull/25935. cc @josibake
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063589430)
Note previous discussion / change in https://github.com/bitcoin/bitcoin/pull/25935. cc @josibake
💬 willcl-ark commented on pull request "Move bitcoin.conf to /share/examples dir in releases":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063599267)
Thanks @fanquake
> it's more correct to have it in the root directory to 1) make it easier for the user to find it and 2) be clear that it is not an example config but a full config
1. IMO people can find it just fine in share/examples, in fact I'd be more likely to `ls` that dir looking for it personally...
2. Would be happy to meet @josibake halfway and classify it as a "full example config" 😄
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063599267)
Thanks @fanquake
> it's more correct to have it in the root directory to 1) make it easier for the user to find it and 2) be clear that it is not an example config but a full config
1. IMO people can find it just fine in share/examples, in fact I'd be more likely to `ls` that dir looking for it personally...
2. Would be happy to meet @josibake halfway and classify it as a "full example config" 😄
💬 laanwj commented on pull request "Move bitcoin.conf to /share/examples dir in releases":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063608628)
i think the point that it isn't actually used is a good one, and it would be good to make that clear, i see two options for that:
- Move it to directory called `example` (maybe better than `share/example`?)
- Put `example` in the file name
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063608628)
i think the point that it isn't actually used is a good one, and it would be good to make that clear, i see two options for that:
- Move it to directory called `example` (maybe better than `share/example`?)
- Put `example` in the file name
🤔 tdb3 reviewed a pull request: "rpc: return warnings as an array instead of just a single one"
(https://github.com/bitcoin/bitcoin/pull/29845#pullrequestreview-2008638781)
ltgm.
cr re ACK for 7cad21a8bbd1e0b64a5e586b405c8619b9e701a5
(https://github.com/bitcoin/bitcoin/pull/29845#pullrequestreview-2008638781)
ltgm.
cr re ACK for 7cad21a8bbd1e0b64a5e586b405c8619b9e701a5
🤔 furszy reviewed a pull request: "refactor: interfaces, make 'createTransaction' less error-prone "
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-2008661209)
Rebased after #806 merge.
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-2008661209)
Rebased after #806 merge.
📝 furszy converted_to_draft a pull request: "index: blockfilter initial sync speedup, parallelize process"
(https://github.com/bitcoin/bitcoin/pull/26966)
The current procedure for building the block filter index involves processing filters one at a time;
Reading blocks, undo data, and previous headers from disk sequentially.
This PR introduces a new mechanism to perform the work concurrently. Dividing the filters
generation workload among a pool of workers that can be configured by the user,
significantly increasing the speed of the index construction process.
The same concurrent processing model has been applied to the transactions inde
...
(https://github.com/bitcoin/bitcoin/pull/26966)
The current procedure for building the block filter index involves processing filters one at a time;
Reading blocks, undo data, and previous headers from disk sequentially.
This PR introduces a new mechanism to perform the work concurrently. Dividing the filters
generation workload among a pool of workers that can be configured by the user,
significantly increasing the speed of the index construction process.
The same concurrent processing model has been applied to the transactions inde
...
💬 alfonsoromanz commented on pull request "Bugfix - don't allow multiple dialogs for same tx in TransactionView":
(https://github.com/bitcoin-core/gui/pull/817#discussion_r1570584795)
In Mac, it seems that `activateWindow()` only makes the window active, i.e: the title of the dialog gets highlighted and gets the keyboard input focus. The `raise()` function brings the dialog on top. So, at least in Mac, it makes sense to keep both IMO
(https://github.com/bitcoin-core/gui/pull/817#discussion_r1570584795)
In Mac, it seems that `activateWindow()` only makes the window active, i.e: the title of the dialog gets highlighted and gets the keyboard input focus. The `raise()` function brings the dialog on top. So, at least in Mac, it makes sense to keep both IMO
💬 mzumsande commented on pull request "test: Fix intermittent issue in p2p_handshake.py":
(https://github.com/bitcoin/bitcoin/pull/29898#discussion_r1570597336)
oh - I didn't know about that, interesting!
Another thing we could do is to not only do `peer.wait_for_disconnect()` but also wait until the bitcoind side has disconnected - something like `self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == X)`. Haven't tried that though.
(https://github.com/bitcoin/bitcoin/pull/29898#discussion_r1570597336)
oh - I didn't know about that, interesting!
Another thing we could do is to not only do `peer.wait_for_disconnect()` but also wait until the bitcoind side has disconnected - something like `self.wait_until(lambda: len(self.nodes[0].getpeerinfo()) == X)`. Haven't tried that though.
👍 ryanofsky approved a pull request: "index: race fix, lock cs_main while 'm_synced' is subject to change"
(https://github.com/bitcoin/bitcoin/pull/29867#pullrequestreview-2008749979)
Code review ACK 65951e0418c53cbbf30b9ee85e24ccaf729088a1
(https://github.com/bitcoin/bitcoin/pull/29867#pullrequestreview-2008749979)
Code review ACK 65951e0418c53cbbf30b9ee85e24ccaf729088a1
💬 brunoerg commented on issue "Wallet fuzzing tracking issue":
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2063768494)
> Coverage is nice, but if the fuzz test never finds any real user-facing issues, it will be useless. I don't think any wallet fuzz target found a real issue yet?
I think we found a real issue with the coin selection target after the rework. But yes, it would be the only one.
> Coverage is nice, but if the fuzz test never finds any real user-facing issues, it will be useless. I don't think any wallet fuzz target found a real issue yet? Obviously coverage is the first step toward adding reg
...
(https://github.com/bitcoin/bitcoin/issues/29901#issuecomment-2063768494)
> Coverage is nice, but if the fuzz test never finds any real user-facing issues, it will be useless. I don't think any wallet fuzz target found a real issue yet?
I think we found a real issue with the coin selection target after the rework. But yes, it would be the only one.
> Coverage is nice, but if the fuzz test never finds any real user-facing issues, it will be useless. I don't think any wallet fuzz target found a real issue yet? Obviously coverage is the first step toward adding reg
...
💬 josibake commented on pull request "Move bitcoin.conf to /share/examples dir in releases":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063776109)
@laanwj @willcl-ark for context, the original goals of having this file were:
1. Make it clear that it was _not_ an example, i.e. nothing in this file is pre-filled for the user, unlike our old bitcoin.conf which did have an example config which was often outdated
2. Make it easy to discover, to deter users from going to some third party for a bitcoin.conf generator (e.g. lopps website for example) or worse, getting a `bitcoin.conf` from "someone on the internet"
For 1, considering that w
...
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063776109)
@laanwj @willcl-ark for context, the original goals of having this file were:
1. Make it clear that it was _not_ an example, i.e. nothing in this file is pre-filled for the user, unlike our old bitcoin.conf which did have an example config which was often outdated
2. Make it easy to discover, to deter users from going to some third party for a bitcoin.conf generator (e.g. lopps website for example) or worse, getting a `bitcoin.conf` from "someone on the internet"
For 1, considering that w
...
👍 alfonsoromanz approved a pull request: "Bugfix on TransactionsView - Disable if privacy mode is set during wallet selection"
(https://github.com/bitcoin-core/gui/pull/815#pullrequestreview-2008812232)
Tested ACK d3da5025f61534d126fef12f198afb65f3a17897
(https://github.com/bitcoin-core/gui/pull/815#pullrequestreview-2008812232)
Tested ACK d3da5025f61534d126fef12f198afb65f3a17897
💬 fanquake commented on pull request "test: Add large aligned vmov check for mingw":
(https://github.com/bitcoin/bitcoin/pull/29874#issuecomment-2063806688)
Looks like `python-capstone` at least exists in Guix already: https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/engineering.scm#n1820.
(https://github.com/bitcoin/bitcoin/pull/29874#issuecomment-2063806688)
Looks like `python-capstone` at least exists in Guix already: https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/engineering.scm#n1820.
💬 laanwj commented on pull request "test: Add large aligned vmov check for mingw":
(https://github.com/bitcoin/bitcoin/pull/29874#issuecomment-2063819614)
> Looks like python-capstone at least exists in Guix already:
> https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/engineering.scm#n1820.
That's good to know! i'm not that surprised, it's a very popular library for doing binary analysis and reverse engineering kind of things.
i'll have a look at adding it.
(https://github.com/bitcoin/bitcoin/pull/29874#issuecomment-2063819614)
> Looks like python-capstone at least exists in Guix already:
> https://git.savannah.gnu.org/cgit/guix.git/tree/gnu/packages/engineering.scm#n1820.
That's good to know! i'm not that surprised, it's a very popular library for doing binary analysis and reverse engineering kind of things.
i'll have a look at adding it.
📝 fjahr opened a pull request: "refactor: Use our own implementation of urlDecode"
(https://github.com/bitcoin/bitcoin/pull/29904)
Removing dependencies is a general goal of the project and the xz backdoor has been an additional wake up call recently. Libevent shows many of the same symptoms, few maintainers and slow releases. While libevent can not be removed completely over night we should start removing it’s usage where it's possible, ideally with the end goal to removing it completely.
This is a pretty easy win in that direction. The [`evhttp_uridecode` function from libevent](https://github.com/libevent/libevent/blo
...
(https://github.com/bitcoin/bitcoin/pull/29904)
Removing dependencies is a general goal of the project and the xz backdoor has been an additional wake up call recently. Libevent shows many of the same symptoms, few maintainers and slow releases. While libevent can not be removed completely over night we should start removing it’s usage where it's possible, ideally with the end goal to removing it completely.
This is a pretty easy win in that direction. The [`evhttp_uridecode` function from libevent](https://github.com/libevent/libevent/blo
...