💬 hebasto commented on pull request "refactor (tidy): Fixes after enable-debug configure":
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1494267607)
> Ok, so it seems we agree there is no code quality benefit.
To clarify my point, this change _does_ improve code performance when the build is configured with `--enable-debug`.
I'm not saying that this fact justifies this PR sufficiently, but it should be considered, at the very least.
> Is the motivation = CI already failing or wanting to add --enable-debug to it?
I'd avoid it as it will change code paths being parsed now (for example, the `DEBUG_LOCKCONTENTION` macro). Maybe just
...
(https://github.com/bitcoin/bitcoin/pull/27353#issuecomment-1494267607)
> Ok, so it seems we agree there is no code quality benefit.
To clarify my point, this change _does_ improve code performance when the build is configured with `--enable-debug`.
I'm not saying that this fact justifies this PR sufficiently, but it should be considered, at the very least.
> Is the motivation = CI already failing or wanting to add --enable-debug to it?
I'd avoid it as it will change code paths being parsed now (for example, the `DEBUG_LOCKCONTENTION` macro). Maybe just
...
📝 fanquake opened a pull request: "depends: add `NO_HARDEN=` option"
(https://github.com/bitcoin/bitcoin/pull/27406)
Add an option that when passed, will disable hardening options, and pass `--disable-hardening` through to configure. Due to the way we link `libssp` for Windows builds, they now fail (after #27118), if building with depends, and configuring with `--disable-hardening` (Windows is the odd build out here). See: https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1492606272.
This change would add a depends option such that, if someone wants to build with depends, for Windows, without harde
...
(https://github.com/bitcoin/bitcoin/pull/27406)
Add an option that when passed, will disable hardening options, and pass `--disable-hardening` through to configure. Due to the way we link `libssp` for Windows builds, they now fail (after #27118), if building with depends, and configuring with `--disable-hardening` (Windows is the odd build out here). See: https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1492606272.
This change would add a depends option such that, if someone wants to build with depends, for Windows, without harde
...
💬 fanquake commented on pull request "depends: add `NO_HARDEN=` option":
(https://github.com/bitcoin/bitcoin/pull/27406#issuecomment-1494311710)
cc @hebasto @TheCharlatan
(https://github.com/bitcoin/bitcoin/pull/27406#issuecomment-1494311710)
cc @hebasto @TheCharlatan
💬 fanquake commented on pull request "depends: harden libevent":
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1494313712)
> This PR introduced a regression when building with depends and disabled hardening:
Followup for discussion in 27406.
(https://github.com/bitcoin/bitcoin/pull/27118#issuecomment-1494313712)
> This PR introduced a regression when building with depends and disabled hardening:
Followup for discussion in 27406.
📝 dergoegge opened a pull request: "net, refactor: Privatise CNode send queue"
(https://github.com/bitcoin/bitcoin/pull/27407)
The send queue members on `CNode` should not be part of the public interface. This PR makes all of them private and creates a clear interface for the send queue.
The interface after this PR consists of:
* `CNode::PushMessage` for appending a message onto the send queue
* `CNode::SocketSendData` for pushing as many messages from the send queue as possible onto the wire
* `CNode::IsSendQueueEmpty` for checking if the send queue is empty
* (`CNode::TestOnlyClearSendQueue` a test-only utility
...
(https://github.com/bitcoin/bitcoin/pull/27407)
The send queue members on `CNode` should not be part of the public interface. This PR makes all of them private and creates a clear interface for the send queue.
The interface after this PR consists of:
* `CNode::PushMessage` for appending a message onto the send queue
* `CNode::SocketSendData` for pushing as many messages from the send queue as possible onto the wire
* `CNode::IsSendQueueEmpty` for checking if the send queue is empty
* (`CNode::TestOnlyClearSendQueue` a test-only utility
...
🚀 fanquake merged a pull request: "refactor: Extract util/fs from util/system"
(https://github.com/bitcoin/bitcoin/pull/27254)
(https://github.com/bitcoin/bitcoin/pull/27254)
💬 fanquake commented on pull request "compat: move (win) S_* defines into bdb":
(https://github.com/bitcoin/bitcoin/pull/26832#issuecomment-1494353437)
Rebased for #27254.
(https://github.com/bitcoin/bitcoin/pull/26832#issuecomment-1494353437)
Rebased for #27254.
💬 fanquake commented on pull request "build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set":
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-1494357963)
Rebased now that #27381 is resolved.
(https://github.com/bitcoin/bitcoin/pull/25972#issuecomment-1494357963)
Rebased now that #27381 is resolved.
✅ fanquake closed an issue: "Lost"
(https://github.com/bitcoin/bitcoin/issues/27408)
(https://github.com/bitcoin/bitcoin/issues/27408)
:lock: fanquake locked an issue: "Lost"
(https://github.com/bitcoin/bitcoin/issues/27408)
(https://github.com/bitcoin/bitcoin/issues/27408)
💬 furszy commented on issue "`listtransactions` fails to list self-send transactions (for imported descriptor wallet)":
(https://github.com/bitcoin/bitcoin/issues/26096#issuecomment-1494423948)
> For self-send detection to work we need to distinguish between receiving and change addresses. For descriptor wallets we do that by checking `internal` flag on the descriptor. But `internal` flag could only be set on an `active` descriptor. Both your imported descriptors are not `active`, therefore we can't determine which outputs are change and which are receiving.
@S3RK, #25979 aims to fix that.
note: It's currently up-to-date with master but I should revisit it after that many months.
(https://github.com/bitcoin/bitcoin/issues/26096#issuecomment-1494423948)
> For self-send detection to work we need to distinguish between receiving and change addresses. For descriptor wallets we do that by checking `internal` flag on the descriptor. But `internal` flag could only be set on an `active` descriptor. Both your imported descriptors are not `active`, therefore we can't determine which outputs are change and which are receiving.
@S3RK, #25979 aims to fix that.
note: It's currently up-to-date with master but I should revisit it after that many months.
💬 darosior commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1494455306)
Thanks everyone for the interest, finally reviving this. Rebased and modernized the code (use static casts and named parameters, make use of batched RPC calls after #26656).
@naumenkogs
> If we really want to check, we can measure how would fees differ by applying this change to historic blocks (?)
We'd also need historical snapshots of a mempool to do that. If someone has got some and is willing to test, i'd be very interested in seeing the results.
> Otherwise, it would help if @dar
...
(https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1494455306)
Thanks everyone for the interest, finally reviving this. Rebased and modernized the code (use static casts and named parameters, make use of batched RPC calls after #26656).
@naumenkogs
> If we really want to check, we can measure how would fees differ by applying this change to historic blocks (?)
We'd also need historical snapshots of a mempool to do that. If someone has got some and is willing to test, i'd be very interested in seeing the results.
> Otherwise, it would help if @dar
...
💬 darosior commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#discussion_r1156064559)
Resolving this, i don't think we can get rid of this heuristic. https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1494455306 gives a counter example where it would lead to a false positive. Please let me know if you disagree.
(https://github.com/bitcoin/bitcoin/pull/25380#discussion_r1156064559)
Resolving this, i don't think we can get rid of this heuristic. https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1494455306 gives a counter example where it would lead to a false positive. Please let me know if you disagree.
💬 Sjors commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1155886202)
8cd77302305a2387368aabe8744b1859caf9956a: what do you mean by this?
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1155886202)
8cd77302305a2387368aabe8744b1859caf9956a: what do you mean by this?
💬 Sjors commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1155907956)
8cd77302305a2387368aabe8744b1859caf9956a: paging @ryanofsky: do you think we should move all this complexity here to the interface?
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1155907956)
8cd77302305a2387368aabe8744b1859caf9956a: paging @ryanofsky: do you think we should move all this complexity here to the interface?
💬 Sjors commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156018577)
Here's a commit to handle the error: https://github.com/Sjors/bitcoin/commit/5711f8274b91a278dd9307972e6985dc96ba56cb
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156018577)
Here's a commit to handle the error: https://github.com/Sjors/bitcoin/commit/5711f8274b91a278dd9307972e6985dc96ba56cb
💬 Sjors commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1155888778)
8cd77302305a2387368aabe8744b1859caf9956a: This seems more consistent with the style above:
```cpp
m_coin_control->m_allow_other_inputs = !m_coin_control->HasSelected();
```
Alternatively you could do `*m_coin_control` at the top of the function.
Maybe also add an `assert(m_coin_control)` at the top.
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1155888778)
8cd77302305a2387368aabe8744b1859caf9956a: This seems more consistent with the style above:
```cpp
m_coin_control->m_allow_other_inputs = !m_coin_control->HasSelected();
```
Alternatively you could do `*m_coin_control` at the top of the function.
Maybe also add an `assert(m_coin_control)` at the top.
💬 furszy commented on pull request "reduce cs_main scope, guard block index 'nFile' under a local mutex":
(https://github.com/bitcoin/bitcoin/pull/27006#issuecomment-1494553264)
Hey dergoegge, thanks for the feedback.
> Approach NACK for adding new globals. It is not worth going with a worse design to "rush in" an improvement for a feature that may or may not happen ([#26966](https://github.com/bitcoin/bitcoin/pull/26966)). I would prefer to go with a more sustainable approach from the get go, otherwise this will need to be refactored in the long run.
No rush here.
[#26966](https://github.com/bitcoin/bitcoin/pull/26966) is only one clear use case. It isn’t the ma
...
(https://github.com/bitcoin/bitcoin/pull/27006#issuecomment-1494553264)
Hey dergoegge, thanks for the feedback.
> Approach NACK for adding new globals. It is not worth going with a worse design to "rush in" an improvement for a feature that may or may not happen ([#26966](https://github.com/bitcoin/bitcoin/pull/26966)). I would prefer to go with a more sustainable approach from the get go, otherwise this will need to be refactored in the long run.
No rush here.
[#26966](https://github.com/bitcoin/bitcoin/pull/26966) is only one clear use case. It isn’t the ma
...
💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156147572)
Basically to follow the same procedure as the send process (`SendCoinsDialog::PrepareSendText`), it's is at line 288-290.
Currently, the GUI disables the automatic selection of "other inputs" when the user manually selected coins (the process will only use those). But, in the future, that could also be customizable. The user might want to let the wallet add new inputs, even when the user had selected some, if there aren't enough funds to cover for the tx target amount.
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156147572)
Basically to follow the same procedure as the send process (`SendCoinsDialog::PrepareSendText`), it's is at line 288-290.
Currently, the GUI disables the automatic selection of "other inputs" when the user manually selected coins (the process will only use those). But, in the future, that could also be customizable. The user might want to let the wallet add new inputs, even when the user had selected some, if there aren't enough funds to cover for the tx target amount.