💬 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

💬 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
...

💬 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.

💬 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.

💬 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.)

👍 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
...

💬 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?).

💬 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). "
```

💬 pablomartin4btc commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1411359535)
+1 (a7240eb99dc792fdf3a6f5aeb93c40c42a8cdc16)

💬 pablomartin4btc commented on pull request "p2p: Allow whitelisting outgoing connections":
(https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1411286326)
nit, if you have to re-touch and it makes sense to you:

```suggestion
"Specify multiple permissions separated by commas (default: download,noban,mempool,relay,in). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
```

💬 andrewtoth commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#issuecomment-1834670816)
@kashifs make sure to wait for the log line `initload thread exit` to be printed in the debug.log file when restarting bitcoind before running the benchmarks.

For RPC, does the following command return block data if you enter `password` when prompted for the password?
```
curl --user user --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"]}' -H 'content-type: text/plain;' http://127.0.0.1:83
...

📝 ishaanam opened a pull request: "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs"
(https://github.com/bitcoin/bitcoin/pull/28979)
This PR:
- Adds a functional test that `sendall` spends unconfirmed change
- Adds a functional test that `sendall` spends regular unconfirmed inputs when specified by user
- Adds ancestor aware funding to `sendall` by using `GetEffectiveValue` for `COutputs`

To-Do
- Add test for ancestor aware funding in `sendall`

💬 jonatack commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1411397167)
Suggest running `./test/lint/lint-whitespace.py` locally on your changes, the lint CI needs appeasing.

🤔 jonatack reviewed a pull request: "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs"
(https://github.com/bitcoin/bitcoin/pull/28979#pullrequestreview-1758644811)
Concept ACK

💬 jonatack commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#issuecomment-1834712813)
The code changes cause this test failure for me locally:

```
m'.
jon|(fb075bab8c9...):~/bitcoin/bitcoin$ ./test/functional/wallet_fundrawtransaction.py --legacy-wallet
2023-11-30T23:17:02.968000Z TestFramework (INFO): PRNG seed is: 4425912909580482609
.../...
2023-11-30T23:17:20.070000Z TestFramework (INFO): Test fundrawtxn with locked wallet and hardened derivation
2023-11-30T23:17:21.869000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent cal
...

👍 stickies-v approved a pull request: "rpc: keep `.cookie` file if it was not generated"
(https://github.com/bitcoin/bitcoin/pull/28784#pullrequestreview-1758684529)
ACK 7cb9367157eb42ee06bc6fa024522cc14a80138d

💬 stickies-v commented on pull request "rpc: keep `.cookie` file if it was not generated":
(https://github.com/bitcoin/bitcoin/pull/28784#discussion_r1411417065)
nit: can be done with a bit less nesting

<details>
<summary>git diff on 7cb9367157</summary>

```diff
diff --git a/src/rpc/request.cpp b/src/rpc/request.cpp
index b7acd62ee3..ad9f244f12 100644
--- a/src/rpc/request.cpp
+++ b/src/rpc/request.cpp
@@ -133,11 +133,10 @@ bool GetAuthCookie(std::string *cookie_out)

void DeleteAuthCookie()
{
+ // Don't delete the cookie file if it wasn't generated by this process
+ if (!g_generated_cookie) return;
try {
- if (g_g
...

🤔 furszy reviewed a pull request: "wallet: Cleanup accidental encryption keys in watchonly wallets"
(https://github.com/bitcoin/bitcoin/pull/28724#pullrequestreview-1758695779)
As the process of reading wallet records from db disregards unknown record types, what if the encryption key is ever used for something else?. Wouldn't that mean that opening such wallet on any previous version (post this PR) would erase the encryption key forever?

The erase process can only verify the private keys disabled flag and whether there is any crypted key or not. But it cannot verify if the encryption key was used for something else.

Guessing that in the case of adding a new type
...

🤔 furszy reviewed a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1758715245)
ha, I made the same change two hours ago. https://github.com/furszy/bitcoin-core/commit/193e0c9c7ffe34ab2e8a4f5b95f66ecc4bac2791. Will review.

💬 mzumsande commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1411429030)
This is not continuous, it only schedules it once more after the initial execution. I think that you meant to use `scheduleEvery`.