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

📝 crypto-perry opened a pull request: "Qrwit nonbc"
(https://github.com/bitcoin/bitcoin/pull/30375)
This is a draft implementation of the scheme described in this (research paper)[https://royalsocietypublishing.org/doi/10.1098/rsos.180410] which introduces a commit delay reveal scheme for safely transitioning ECDSA secured UTXOs to a quantum resistant signature scheme.

👋 crypto-perry's pull request is ready for review: "Qrwit nonbc"
(https://github.com/bitcoin/bitcoin/pull/30375)

📝 crypto-perry converted_to_draft a pull request: "QRWit Implementation Proposal"
(https://github.com/bitcoin/bitcoin/pull/30375)
This is a draft implementation of the scheme described in this [research paper](https://royalsocietypublishing.org/doi/10.1098/rsos.180410) which introduces a commit delay reveal scheme for safely transitioning ECDSA secured UTXOs to a quantum resistant signature scheme.

👍 tdb3 approved a pull request: "#27307 follow-up: update mempool conflict tests + docs"
(https://github.com/bitcoin/bitcoin/pull/30365#pullrequestreview-2152389534)
ACK 7d55796c530f891493302059c6200d4a606c1ca9
Good follow up. Ran `wallet_conflicts` locally without issue.

✅ achow101 closed a pull request: "QRWit Implementation Proposal"
(https://github.com/bitcoin/bitcoin/pull/30375)

💬 achow101 commented on pull request "QRWit Implementation Proposal":
(https://github.com/bitcoin/bitcoin/pull/30375#issuecomment-2201418712)
Thank you for your contribution, however consensus changes need to be discussed on the mailing list first, and preferably have a BIP, before implementation here should begin being discussed.

💬 tdb3 commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2201432813)
> Hmm, not sure if that is expected behavior or not for gettxoutsetinfo. If the block is invalidated, seems counterintuitive to consider it as a hash_or_height for calculating txout set info. Not sure what piece is missing here, but will dig more.

Following up:

- Generated a chain of length 101.
- Invalidated block 2.
- `gettxoutsetinfo` with no arguments returns info for the chain (of height 1) as expected.
- `gettxoutsetinfo` with a block height >= 2 returns the expected error mes
...

🤔 tdb3 reviewed a pull request: "net: Allow -proxy=[::1] on nodes with IPV6 lo only"
(https://github.com/bitcoin/bitcoin/pull/30245#pullrequestreview-2152531616)
Concept ACK
Good find. Left a couple of comments.

💬 tdb3 commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1661731868)
Instead of using 0 as the first argument, might be better to remove only `AI_ADDRCONFIG`, e.g. something like
```c++
callgetaddrinfo(ai_flags & (~AI_ADDRCONFIG), true);
```

💬 tdb3 commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1661728878)
This makes sense to me as well and it might make for a significantly smaller code change.