💬 achow101 commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2200850676)
ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce

🤔 furszy reviewed a pull request: "#27307 follow-up: update mempool conflict tests + docs"
(https://github.com/bitcoin/bitcoin/pull/30365#pullrequestreview-2152124441)
utACK 7d55796c530

💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2200887728)
> > Do blocks mined with the exception count towards the total work according to difficulty-1 or the actual difficulty? If it’s the latter, in testnet3 you could just invalidate the last block in the previous difficulty period with a difficulty-1 block. If it’s the former, I agree, the absence of this attack may indicate that I’m worrying too much.
>
> (Side-note: I first read difficulty-1 as "difficulty minus 1" and got a bit confused but I think you mean the same as "difficulty=1")
>
> I
...

🤔 achow101 reviewed a pull request: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2152124552)
Concept ACK

I'm not a huge fan of all the boilerplate and wrappers that this ends up adding, but I also don't see a better way to do this.

In `ApplyMigrationData`, it looks like we're creating batchs for the watchonly and solvable wallets multiple times. Those could be consolidated in this PR as well.

💬 achow101 commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1661464266)
In 4e72821e1961b58559f49a4746bc8d15d116f6b7 "wallet: ApplyMigrationData, group all db txs into a single atomic operation"

This could be its own commit.

💬 achow101 commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#discussion_r1661483289)
In faa9a1fcd05341c1cbe9b2e2257c1c0248b2a4d4 "rpc: Avoid getchaintxstats invalid results"

typo: "start end end". I think that's supposed to be "start and end"

```suggestion
"Only returned if \"window_block_count\" is > 0 and if txcount exists for the start and end of the window."},
```

✅ ceffikhan closed a pull request: "test: fix debug log assertion in p2p_i2p_ports test"
(https://github.com/bitcoin/bitcoin/pull/30345)

💬 ceffikhan commented on pull request "test: fix debug log assertion in p2p_i2p_ports test":
(https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200940397)
closing PR for now

💬 paplorinc commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2200977622)
> Ok, the reason for https://github.com/bitcoin/bitcoin/pull/7656#issue-139438690 was an improvement in listunspent. Seems fine, if this is still the case. But this will need to be checked first.

I've created 10k blocks in regtest and measured the performance of `listunspent` before and after:

```bash
killall bitcoind 2>/dev/null || true
./src/bitcoind -regtest -daemon
./src/bitcoin-cli -regtest createwallet "test_wallet"
./src/bitcoin-cli -regtest generatetoaddress 10000 $(./src/bitco
...

💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#issuecomment-2200986730)
> Also, "protocol.h" in the first commit message body seems wrong or out-of-date here: `bringing the error message in line with the definition established in protocol.h ("error when there are multiple wallets loaded")`. I'm not sure, but did you mean "src/wallet/rpc/util.cpp" instead?

The reference to "`src/rpc/protocol.h`" was intended to clarify the context of the error message alignment, specifically in the section of the code where the RPC error code "not wallet specified" is defined (`en
...

💬 pablomartin4btc commented on pull request "cli: improve error message on multiwallet and add validation to cli-side commands":
(https://github.com/bitcoin/bitcoin/pull/26990#discussion_r1661528309)
Ok, thanks for checking, I'll work on it and will incorporate full error message in the functional test.

💬 brunoerg commented on issue "Intermittent issue in p2p_i2p_ports.py AssertionError: [node 0] Expected messages "['Error connecting to [...].b32.i2p:0: Cannot connect to 127.0.0.1:60000']" does not partially match log:":
(https://github.com/bitcoin/bitcoin/issues/30030#issuecomment-2201034319)
See https://github.com/bitcoin/bitcoin/pull/30345#issuecomment-2200659833

It seems there is something listening at port 60000 in parallel.

💬 fjahr commented on pull request "#27307 follow-up: update mempool conflict tests + docs":
(https://github.com/bitcoin/bitcoin/pull/30365#issuecomment-2201099336)
ACK 7d55796c530f891493302059c6200d4a606c1ca9

The CI failures are related to #30368, not this change.

💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661566826)
(Now grepping "AddKeyPubKeyInner(const CKey& key, const CPubKey& pubkey)" gives 1 result.
grepping "AddKeyPubKeyInner(const CKey& key, const CPubKey &pubkey)" gives 3 results.

It would be nice to align style within the same class between .CPP/.H).

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661578955)
Not going to change existing code, especially code that will be deleted, to match style

💬 cbergqvist commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1661582339)
(Agreed, but then I would skip doing the suggested change in the .CPP file).

👍 cbergqvist approved a pull request: "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB"
(https://github.com/bitcoin/bitcoin/pull/26596#pullrequestreview-2152310896)
ACK 8ce3739edbcf6437bf2695087e0ebe8c633df19b

`git range-diff 9700c217~5..9700c217 8ce3739~5..8ce3739` - not the easiest read, also used GH [compare](https://github.com/bitcoin/bitcoin/compare/9700c21704ddad4be07df44393e39057fa386c56..8ce3739edbcf6437bf2695087e0ebe8c633df19b).

Passed `make check` & `test/functional/test_runner.py` including `p2p_handshake.py` which [failed on CI](https://github.com/bitcoin/bitcoin/pull/26596/checks?check_run_id=26906002421).

Only one non-critical reserva
...

🤔 cbergqvist requested changes to a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2152330809)
NACK ffe9b274984a351716348a6c99df19c391bfdc8e

The current solution means most test nodes in the functional test suite will now bind to an onion port even though the vast majority are not testing Tor. My proposed solution in 6b785873557696cc611d58fdcac5a3d54622082c did not have that behavior. Maybe you could add a custom flag on the Python test framework level that can be opted in to by tests that require it?

💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1661599469)
Did you skip this in the ffe9b274984a351716348a6c99df19c391bfdc8e force-push because you weren't modifying this specific file or did you forget?

👍 tdb3 approved a pull request: "Revert "test: p2p: check that connecting to ourself leads to disconnect""
(https://github.com/bitcoin/bitcoin/pull/30374#pullrequestreview-2152339467)
ACK 9ec2c53701a391629b55aeb2804e8060d2c453a4
Glad this test found a bug!
Makes sense to revert this out until the issue is fixed.
Ran the test locally.