💬 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.
💬 ismaelsadeeq commented on pull request "test: various `converttopsbt` check cleanups in rpc_psbt.py":
(https://github.com/bitcoin/bitcoin/pull/27325#issuecomment-1494594347)
tested ACK afc2dd54848fa70ed408ae259420ff8648f21efc
I ran the test and it passed.
(https://github.com/bitcoin/bitcoin/pull/27325#issuecomment-1494594347)
tested ACK afc2dd54848fa70ed408ae259420ff8648f21efc
I ran the test and it passed.
💬 furszy commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156170345)
The `m_allow_other_inputs` flag is different to the `fAllowWatchOnly` flag.
`fAllowWatchOnly` set is actually redundant here as `privateKeysDisabled()` and `hasExternalSigner()` are wallet constants. It can be set only once, when the wallet is set in the GUI and don't be re-set again.
And, at the contraire, the `m_allow_other_inputs` flag can change as it depends on whether the user manually selected coins or not, which could be modified at any time. Thus why I would try to not set the coi
...
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156170345)
The `m_allow_other_inputs` flag is different to the `fAllowWatchOnly` flag.
`fAllowWatchOnly` set is actually redundant here as `privateKeysDisabled()` and `hasExternalSigner()` are wallet constants. It can be set only once, when the wallet is set in the GUI and don't be re-set again.
And, at the contraire, the `m_allow_other_inputs` flag can change as it depends on whether the user manually selected coins or not, which could be modified at any time. Thus why I would try to not set the coi
...
📝 ryanofsky opened a pull request: "Make GUI and CLI tools use the same datadir"
(https://github.com/bitcoin/bitcoin/pull/27409)
**This is based on #27302.** The non-base commits are:
- [`cb2f82421d5` bitcoin-wallet: make bitcoin-wallet tool load config file](https://github.com/bitcoin/bitcoin/pull/27409/commits/cb2f82421d557cfe835b686a6f25965ee190e329)
- [`04a7fca01d3` init: Allow bitcoin default datadir to point at an external datadir](https://github.com/bitcoin/bitcoin/pull/27409/commits/04a7fca01d330bd9d6f33bf01dd380a8f0f36604)
- [`3c2b5edbfbf` Make GUI and CLI tools use the same datadir](https://github.com/bitco
...
(https://github.com/bitcoin/bitcoin/pull/27409)
**This is based on #27302.** The non-base commits are:
- [`cb2f82421d5` bitcoin-wallet: make bitcoin-wallet tool load config file](https://github.com/bitcoin/bitcoin/pull/27409/commits/cb2f82421d557cfe835b686a6f25965ee190e329)
- [`04a7fca01d3` init: Allow bitcoin default datadir to point at an external datadir](https://github.com/bitcoin/bitcoin/pull/27409/commits/04a7fca01d330bd9d6f33bf01dd380a8f0f36604)
- [`3c2b5edbfbf` Make GUI and CLI tools use the same datadir](https://github.com/bitco
...
💬 ryanofsky commented on pull request "Make GUI and CLI tools use the same datadir":
(https://github.com/bitcoin/bitcoin/pull/27409#issuecomment-1494623027)
This is a draft PR and not fully implemented. Just opening it to share progress since it's taking longer than I expected to implement correctly
(https://github.com/bitcoin/bitcoin/pull/27409#issuecomment-1494623027)
This is a draft PR and not fully implemented. Just opening it to share progress since it's taking longer than I expected to implement correctly
💬 TheCharlatan commented on pull request "refactor: Move chain names to the util library":
(https://github.com/bitcoin/bitcoin/pull/27294#issuecomment-1494624073)
Updated 603f739947ce29d50affe5451cdae92c1744fab2 -> f3c57bf4654599a4343cee3a1d2ffa7026a8ade7 ([kernelChainName_2](https://github.com/TheCharlatan/bitcoin/commits/kernelChainName_2) -> [kernelChainName_3](https://github.com/TheCharlatan/bitcoin/commits/kernelChainName_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelChainName_2..kernelChainName_3)).
(https://github.com/bitcoin/bitcoin/pull/27294#issuecomment-1494624073)
Updated 603f739947ce29d50affe5451cdae92c1744fab2 -> f3c57bf4654599a4343cee3a1d2ffa7026a8ade7 ([kernelChainName_2](https://github.com/TheCharlatan/bitcoin/commits/kernelChainName_2) -> [kernelChainName_3](https://github.com/TheCharlatan/bitcoin/commits/kernelChainName_3), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelChainName_2..kernelChainName_3)).
💬 Sjors commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156186159)
I see. I think it's better to drop that comment and instead open a GUI issue. The checkbox could say "Add more coins as needed" to match the send RPC behaviour. And it can point to the line `!coin_control.HasSelected();` that needs to be changed to implement this.
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156186159)
I see. I think it's better to drop that comment and instead open a GUI issue. The checkbox could say "Add more coins as needed" to match the send RPC behaviour. And it can point to the line `!coin_control.HasSelected();` that needs to be changed to implement this.
💬 Sjors commented on pull request "wallet, gui: bugfix, getAvailableBalance skips selected coins":
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156188348)
Ah, maybe add a comment:
```cpp
// Copy to avoid modifying m_coin_control
```
(https://github.com/bitcoin/bitcoin/pull/26699#discussion_r1156188348)
Ah, maybe add a comment:
```cpp
// Copy to avoid modifying m_coin_control
```
👍 brunoerg approved a pull request: "test: refactor: replace unnecessary `BytesIO` uses"
(https://github.com/bitcoin/bitcoin/pull/27389)
crACK f842ed9a40c0db656b86f85e84dd4978865cc0a0
(https://github.com/bitcoin/bitcoin/pull/27389)
crACK f842ed9a40c0db656b86f85e84dd4978865cc0a0
💬 mzumsande commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1156247998)
I agree, see below.
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1156247998)
I agree, see below.
👍 aureleoules approved a pull request: "test: refactor: replace unnecessary `BytesIO` uses"
(https://github.com/bitcoin/bitcoin/pull/27389)
ACK f842ed9a40c0db656b86f85e84dd4978865cc0a0 - It seems that these are the only instances that can be changed and it simplifies test code.
(https://github.com/bitcoin/bitcoin/pull/27389)
ACK f842ed9a40c0db656b86f85e84dd4978865cc0a0 - It seems that these are the only instances that can be changed and it simplifies test code.
💬 martinus commented on pull request "util: Use steady clock instead of system clock to measure durations":
(https://github.com/bitcoin/bitcoin/pull/27405#discussion_r1156279841)
I think this should be `20s`. I guess in this case it might be safer to write
```cpp
std::chrono::milliseconds g_socks5_recv_timeout = 20s;
```
because just
```cpp
std::chrono::milliseconds g_socks5_recv_timeout = 20;
```
would give you a compile error
(https://github.com/bitcoin/bitcoin/pull/27405#discussion_r1156279841)
I think this should be `20s`. I guess in this case it might be safer to write
```cpp
std::chrono::milliseconds g_socks5_recv_timeout = 20s;
```
because just
```cpp
std::chrono::milliseconds g_socks5_recv_timeout = 20;
```
would give you a compile error
💬 sdaftuar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1494762525)
> however, the OP of that post also states:
>> This is still somewhat incomplete protection because our outbound
peers could be down but not timed out or might all be on 'local'
networks (although the requirement for multiple netgroups helps).
@amitiuttarwar Thanks for finding this, I hadn't thought about the issue of wanting to distinguish between local and non-local IPV4 networks for the purposes of detecting whether a node's network connectivity is down.
This means my suggestion of
...
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1494762525)
> however, the OP of that post also states:
>> This is still somewhat incomplete protection because our outbound
peers could be down but not timed out or might all be on 'local'
networks (although the requirement for multiple netgroups helps).
@amitiuttarwar Thanks for finding this, I hadn't thought about the issue of wanting to distinguish between local and non-local IPV4 networks for the purposes of detecting whether a node's network connectivity is down.
This means my suggestion of
...
👋 pinheadmz's pull request is ready for review: "net: support unix domain sockets for -proxy and -onion"
(https://github.com/bitcoin/bitcoin/pull/27375)
(https://github.com/bitcoin/bitcoin/pull/27375)
💬 theuni commented on pull request "depends: add `NO_HARDEN=` option":
(https://github.com/bitcoin/bitcoin/pull/27406#discussion_r1156323737)
Nit: spelling
(https://github.com/bitcoin/bitcoin/pull/27406#discussion_r1156323737)
Nit: spelling
💬 TheBlueMatt commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1494803900)
> Downloading to a clean dir and using a wildcard should still work for tooling I think. @TheBlueMatt ?
Not sure exactly what you mean here - part of the goal is to have tooling to verify all the binaries sitting in a folder? If you can "download" all of them in one go from `file:///` I guess that works too?
(https://github.com/bitcoin/bitcoin/pull/27358#issuecomment-1494803900)
> Downloading to a clean dir and using a wildcard should still work for tooling I think. @TheBlueMatt ?
Not sure exactly what you mean here - part of the goal is to have tooling to verify all the binaries sitting in a folder? If you can "download" all of them in one go from `file:///` I guess that works too?
📝 mzumsande opened a pull request: "p2p: Restrict self-advertisements with privacy networks to avoid fingerprinting"
(https://github.com/bitcoin/bitcoin/pull/27411)
The current logic for self-advertisements works such that we detect as many local addresses as we can, and then, using the scoring matrix from `CNetAddr::GetReachabilityFrom()`, self-advertise with the address that fits best to our peer.
It is in general not hard for our peers to distinguish our self-advertisements from other addrs we send them, because we self-advertise every ~24h and because the first addr we send over a connection is likely our self-advertisement.
`GetReachabilityFrom()`
...
(https://github.com/bitcoin/bitcoin/pull/27411)
The current logic for self-advertisements works such that we detect as many local addresses as we can, and then, using the scoring matrix from `CNetAddr::GetReachabilityFrom()`, self-advertise with the address that fits best to our peer.
It is in general not hard for our peers to distinguish our self-advertisements from other addrs we send them, because we self-advertise every ~24h and because the first addr we send over a connection is likely our self-advertisement.
`GetReachabilityFrom()`
...