💬 jonatack commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1834473406)
Concept ACK based on today's IRC discussion https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-11-30#986727.
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1834473406)
Concept ACK based on today's IRC discussion https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2023-11-30#986727.
💬 achow101 commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1411216955)
Will add a comment if I re-touch
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1411216955)
Will add a comment if I re-touch
💬 achow101 commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1411217384)
I don't think it would be useful to log them.
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1411217384)
I don't think it would be useful to log them.
💬 achow101 commented on pull request "wallet: Cleanup accidental encryption keys in watchonly wallets":
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1411218639)
I think this is fine as is. It can be updated in the future if it becomes outdated.
(https://github.com/bitcoin/bitcoin/pull/28724#discussion_r1411218639)
I think this is fine as is. It can be updated in the future if it becomes outdated.
💬 jonatack commented on pull request "net: additional disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-1834503005)
Concept ACK. I'd find it helpful to also log the addrport if `fLogIPs` is true, along with the network (for instance, because IPv6 addresses look the same as CJDNS ones), e.g. something like `net=..., peeraddr=...,`
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-1834503005)
Concept ACK. I'd find it helpful to also log the addrport if `fLogIPs` is true, along with the network (for instance, because IPv6 addresses look the same as CJDNS ones), e.g. something like `net=..., peeraddr=...,`
👍 kristapsk approved a pull request: "rpc: keep `.cookie` file if it was not generated"
(https://github.com/bitcoin/bitcoin/pull/28784#pullrequestreview-1758419193)
re-ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d
(https://github.com/bitcoin/bitcoin/pull/28784#pullrequestreview-1758419193)
re-ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d
📝 achow101 opened a pull request: "Migrate blank"
(https://github.com/bitcoin/bitcoin/pull/28976)
Blank wallets (wallets without any keys are scripts) are being detected as already being descriptor wallets even though they are not. This is because the check for whether a wallet is already a descriptor wallet uses the presence of a `LegacyScriptPubKeyMan` which is only setup when keys or scripts are found. This PR resolves this issue by checking for the descriptor wallet flag instead and subsequently skipping the keys and scripts part of migration for blank wallets.
Fixes the issue mention
...
(https://github.com/bitcoin/bitcoin/pull/28976)
Blank wallets (wallets without any keys are scripts) are being detected as already being descriptor wallets even though they are not. This is because the check for whether a wallet is already a descriptor wallet uses the presence of a `LegacyScriptPubKeyMan` which is only setup when keys or scripts are found. This PR resolves this issue by checking for the descriptor wallet flag instead and subsequently skipping the keys and scripts part of migration for blank wallets.
Fixes the issue mention
...
💬 achow101 commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1834556361)
> While trying to test the first commit, I ran into something odd. It seems that an _empty_ watch-only wallet legacy is treated as a descriptor wallet by `migratewallet`.
This is a separate issue in master. I've opened #28976 to address it.
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1834556361)
> While trying to test the first commit, I ran into something odd. It seems that an _empty_ watch-only wallet legacy is treated as a descriptor wallet by `migratewallet`.
This is a separate issue in master. I've opened #28976 to address it.
📝 murchandamus opened a pull request: "Add Gutter Guard Selector"
(https://github.com/bitcoin/bitcoin/pull/28977)
Does a random selection limited to three more output groups than largest-first selection would select to fund the transaction. If the limit is exceeded during selection, the output group with the lowest effective value is discarded.
Adds a coin selection algorithm that minimizes the weight of the input set while creating change.
Motivations
---
- At high feerates, using unnecessary inputs can [significantly increase the fees](https://bitcoin.stackexchange.com/q/120408/5406)
- Users ar
...
(https://github.com/bitcoin/bitcoin/pull/28977)
Does a random selection limited to three more output groups than largest-first selection would select to fund the transaction. If the limit is exceeded during selection, the output group with the lowest effective value is discarded.
Adds a coin selection algorithm that minimizes the weight of the input set while creating change.
Motivations
---
- At high feerates, using unnecessary inputs can [significantly increase the fees](https://bitcoin.stackexchange.com/q/120408/5406)
- Users ar
...
📝 ryanofsky opened a pull request: "doc: Add multiprocess design doc"
(https://github.com/bitcoin/bitcoin/pull/28978)
Add multiprocess design doc and existing multiprocess documentation into design and usage sections.
Links to rendered markdown:
https://github.com/ryanofsky/bitcoin/blob/pr/ipcdoc/doc/design/multiprocess.md
https://github.com/ryanofsky/bitcoin/blob/pr/ipcdoc/doc/multiprocess.md
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
(https://github.com/bitcoin/bitcoin/pull/28978)
Add multiprocess design doc and existing multiprocess documentation into design and usage sections.
Links to rendered markdown:
https://github.com/ryanofsky/bitcoin/blob/pr/ipcdoc/doc/design/multiprocess.md
https://github.com/ryanofsky/bitcoin/blob/pr/ipcdoc/doc/multiprocess.md
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
💬 fanquake commented on pull request "wallet: Fix migration of blank wallets":
(https://github.com/bitcoin/bitcoin/pull/28976#issuecomment-1834589858)
For backport or no?
(https://github.com/bitcoin/bitcoin/pull/28976#issuecomment-1834589858)
For backport or no?
📝 murchandamus converted_to_draft a pull request: "Add Gutter Guard Selector"
(https://github.com/bitcoin/bitcoin/pull/28977)
Does a random selection limited to three more output groups than largest-first selection would select to fund the transaction. If the limit is exceeded during selection, the output group with the lowest effective value is discarded.
Adds a coin selection algorithm that minimizes the weight of the input set while creating change.
Motivations
---
- At high feerates, using unnecessary inputs can [significantly increase the fees](https://bitcoin.stackexchange.com/q/120408/5406)
- Users ar
...
(https://github.com/bitcoin/bitcoin/pull/28977)
Does a random selection limited to three more output groups than largest-first selection would select to fund the transaction. If the limit is exceeded during selection, the output group with the lowest effective value is discarded.
Adds a coin selection algorithm that minimizes the weight of the input set while creating change.
Motivations
---
- At high feerates, using unnecessary inputs can [significantly increase the fees](https://bitcoin.stackexchange.com/q/120408/5406)
- Users ar
...
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1411316794)
Why not just make a copy of the seeded randomizer instead of accumulating to it all the changes from `m_states`? That way you ensure that a change in the ordering is not going to, potentially, produce a completely different order of the whole `best_peers` set. In this case, the worst that could happen would be that the newly added peer jumps places, and becomes one of the selected/not selected peers, instead of a complete rearrange of the collection
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1411316794)
Why not just make a copy of the seeded randomizer instead of accumulating to it all the changes from `m_states`? That way you ensure that a change in the ordering is not going to, potentially, produce a completely different order of the whole `best_peers` set. In this case, the worst that could happen would be that the newly added peer jumps places, and becomes one of the selected/not selected peers, instead of a complete rearrange of the collection
💬 fanquake commented on issue "Building a wallet with legacy support fails on OpenBSD 7.4":
(https://github.com/bitcoin/bitcoin/issues/28963#issuecomment-1834640327)
> In file included from ../dist/./../cxx/cxx_db.cpp:13:
> ./db_cxx.h:59:10: fatal error: 'iostream.h' file not found
The issue is some code like this in bdb:
```cpp
#ifdef HAVE_CXX_STDHEADERS
#include <iostream>
#include <exception>
#define __DB_STD(x) std::x
#else
#include <iostream.h>
#include <exception.h>
#define __DB_STD(x) x
#endif
```
For some reason, configure fails to detect C++ header availability, and `HAVE_CXX_STDHEADERS` is not defined. Is `g++` and a C++ standard
...
(https://github.com/bitcoin/bitcoin/issues/28963#issuecomment-1834640327)
> In file included from ../dist/./../cxx/cxx_db.cpp:13:
> ./db_cxx.h:59:10: fatal error: 'iostream.h' file not found
The issue is some code like this in bdb:
```cpp
#ifdef HAVE_CXX_STDHEADERS
#include <iostream>
#include <exception>
#define __DB_STD(x) std::x
#else
#include <iostream.h>
#include <exception.h>
#define __DB_STD(x) x
#endif
```
For some reason, configure fails to detect C++ header availability, and `HAVE_CXX_STDHEADERS` is not defined. Is `g++` and a C++ standard
...
💬 achow101 commented on pull request "wallet: Fix migration of blank wallets":
(https://github.com/bitcoin/bitcoin/pull/28976#issuecomment-1834640431)
> For backport or no?
No, I don't think this is a regression, nor is it really a bug that is that important. If someone runs into it, the wallet is completely blank and empty, so they can just delete it and make a new descriptors wallet.
(https://github.com/bitcoin/bitcoin/pull/28976#issuecomment-1834640431)
> For backport or no?
No, I don't think this is a regression, nor is it really a bug that is that important. If someone runs into it, the wallet is completely blank and empty, so they can just delete it and make a new descriptors wallet.
💬 achow101 commented on pull request "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs":
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1834644837)
Figured out the random failure, should be all working now.
(https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1834644837)
Figured out the random failure, should be all working now.
💬 jonatack commented on issue "bitcoin 25.1 regression test failure against sqlite 3.44.1":
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1834656585)
(FWIW, upgraded sqlite from 3.44.1 to 3.44.2. via homebrew, compiled and ran tests, and v25.1 and master are both green.)
(https://github.com/bitcoin/bitcoin/issues/28941#issuecomment-1834656585)
(FWIW, upgraded sqlite from 3.44.1 to 3.44.2. via homebrew, compiled and ran tests, and v25.1 and master are both green.)
👍 pablomartin4btc approved a pull request: "p2p: Allow whitelisting outgoing connections"
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1758467940)
tACK d0e3c62d414d91f34bf9c0991a58139ae9365221
Tested it with `bitcoin-qt` and I'm wondering if later as a follow-up we could add a feature as a "nice to have" to identify the "whitelisted" peers (and it permissions passed) in the Peers tab on the Node window (we [show](https://bitcoinexplorer.org/rpc-browser?method=getpeerinfo) already the `permissions@` on `RPC` `getpeerinfo` but not sure about other details on `-whitelist` or `-whitebind` and if we want to provide that info via `RPC`). Ther
...
(https://github.com/bitcoin/bitcoin/pull/27114#pullrequestreview-1758467940)
tACK d0e3c62d414d91f34bf9c0991a58139ae9365221
Tested it with `bitcoin-qt` and I'm wondering if later as a follow-up we could add a feature as a "nice to have" to identify the "whitelisted" peers (and it permissions passed) in the Peers tab on the Node window (we [show](https://bitcoinexplorer.org/rpc-browser?method=getpeerinfo) already the `permissions@` on `RPC` `getpeerinfo` but not sure about other details on `-whitelist` or `-whitebind` and if we want to provide that info via `RPC`). Ther
...
💬 pablomartin4btc commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1411270076)
Due to this change, should we not update the `-whitebind` documentation in `src/init.cpp`? (and relase notes?).
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1411270076)
Due to this change, should we not update the `-whitebind` documentation in `src/init.cpp`? (and relase notes?).
💬 pablomartin4btc commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1411290808)
nit, since it was not very clear to me at first sight and I saw another [comment](https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275411963) related to this (feel free to change it if you agree with the general idea):
```suggestion
"Additional flags \"in\" and \"out\" are part of [permissions@] and control whether permissions apply to incoming connections and/or outgoing (default: incoming only - e.g. noban,in,out@1.2.3.4). "
```
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1411290808)
nit, since it was not very clear to me at first sight and I saw another [comment](https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1275411963) related to this (feel free to change it if you agree with the general idea):
```suggestion
"Additional flags \"in\" and \"out\" are part of [permissions@] and control whether permissions apply to incoming connections and/or outgoing (default: incoming only - e.g. noban,in,out@1.2.3.4). "
```
💬 pablomartin4btc commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1411359535)
+1 (a7240eb99dc792fdf3a6f5aeb93c40c42a8cdc16)
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1411359535)
+1 (a7240eb99dc792fdf3a6f5aeb93c40c42a8cdc16)