💬 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
...
💬 laanwj commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2063835209)
Concept ACK
yes please... the hooking we do for `URL_DECODE` and `UrlDecodeFn` made me kind of scared, to be honest (in that context), i think can we get rid of that at the same time?
This will also fix @luke-jr's #29654.
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2063835209)
Concept ACK
yes please... the hooking we do for `URL_DECODE` and `UrlDecodeFn` made me kind of scared, to be honest (in that context), i think can we get rid of that at the same time?
This will also fix @luke-jr's #29654.
💬 furszy commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1570731552)
In 4b05c654c1:
Using `m_best_header` shouldn't be working (at least not as you expect) for `invalidateblock`.
During `InvalidateBlock()`, we first disconnect all blocks, then call `CheckBlockIndex` without updating `m_best_header` (this is done later on the flow, when we call `InvalidChainFound()`). So, by the time `CheckBlockIndex` is called, `best_hdr_chain` is loaded with a chain that is not the active one.
Any reason not to use `ActiveTip()` instead of `m_best_header` here?
(https://github.com/bitcoin/bitcoin/pull/28339#discussion_r1570731552)
In 4b05c654c1:
Using `m_best_header` shouldn't be working (at least not as you expect) for `invalidateblock`.
During `InvalidateBlock()`, we first disconnect all blocks, then call `CheckBlockIndex` without updating `m_best_header` (this is done later on the flow, when we call `InvalidChainFound()`). So, by the time `CheckBlockIndex` is called, `best_hdr_chain` is loaded with a chain that is not the active one.
Any reason not to use `ActiveTip()` instead of `m_best_header` here?
💬 furszy commented on pull request "validation: improve performance of CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-2063852471)
@DrahtBot, I just reviewed it..
(https://github.com/bitcoin/bitcoin/pull/28339#issuecomment-2063852471)
@DrahtBot, I just reviewed it..
💬 willcl-ark commented on pull request "Move bitcoin.conf to /share/examples dir in releases":
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063858757)
Ok there seems to be some push back here. Personally, I'd prefer to see files neatly in directories so that folks installing from a tarball directly into `/usr/local` wouldn't have random odds-and-ends ending up in the root there, but happy to conceed on this (perhaps I install binary tarballs [weirdly](https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1895518581)?)
Seems like the path of least resistance is just to append `.example`, so I've done that here now.
> I think all we
...
(https://github.com/bitcoin/bitcoin/pull/29903#issuecomment-2063858757)
Ok there seems to be some push back here. Personally, I'd prefer to see files neatly in directories so that folks installing from a tarball directly into `/usr/local` wouldn't have random odds-and-ends ending up in the root there, but happy to conceed on this (perhaps I install binary tarballs [weirdly](https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1895518581)?)
Seems like the path of least resistance is just to append `.example`, so I've done that here now.
> I think all we
...
💬 TheCharlatan commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2063858822)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29904#issuecomment-2063858822)
Concept ACK