💬 kristapsk commented on pull request "Modify command line help to show support for BIP21 URIs":
(https://github.com/bitcoin-core/gui/pull/752#discussion_r1437157234)
Square brackets mean that URI option is optional. Either use "[URI]" or "Optional URI" below, not "Optional [URI]".
```suggestion
"Optional URI is a bitcoin address in BIP21 URI format.\n";
```

⚠️ kristapsk opened an issue: ""Open bitcoin URI" dialog could give more feedback on what's wrong with BIP21 URI"
(https://github.com/bitcoin-core/gui/issues/784)
### Please describe the feature you'd like to see added.

Currently, when trying to open URI, if it's invalid, clicking "OK" will just colour it red, instead of giving any feedback, what's exactly wrong with URI.


In my test I added unknown required paramter `req-test=1`. Feedback on that would be useful for user, for example, [BIP77 Payjoin v2 draft](https://github.com/bitcoin/bips/pull/1483)
...

💬 asggWa commented on issue "./bitcoin.conf file should not cause confusion with ./datadir/bitcoin.conf":
(https://github.com/bitcoin/bitcoin/issues/29139#issuecomment-1870516002)
$HOME/snap

💬 kristapsk commented on pull request "Update about logo icon (colour) to denote the chain type of the QT instance in About/ Help Message Window/ Dialog":
(https://github.com/bitcoin-core/gui/pull/762#discussion_r1437190084)
nit
```suggestion
const QSize requiredSize(1024, 1024);
```

💬 ybaidiuk commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1870530971)
Ok filter is bad idea because of censoring and disable OP_RETRUN is not an Option,
but we still have free market violation (fee)
People do not pay full price to put data in blockchain, they do abuse of fee-free size of OP_RETRUN.

In this case, will be doing nothing is a good solution.
But i think the best option will be to reduce size of OP_RETURN to 40 bytes like it was before.
It will make fee-abuse more expensive and create stimulation to move data into side chains.

🤔 mzumsande reviewed a pull request: "Change Luke Dashjr seed to dashjr-list-of-p2p-nodes-maybe-malware.us"
(https://github.com/bitcoin/bitcoin/pull/29145#pullrequestreview-1797445282)
@luke-jr : Slightly unrelated to this PR, but looking at the results from all seeders, it seems that yours returns only nodes running old versions `0.21.x` and `22.x`. I didn't get a single result with a newer subversion.
Given the current composition of nodes on the network and that none of the other DNS seeds show a similar pattern, this seems unlikely to just be an unlucky result of a random selection. Could this be a bug in your seeder?

👍 kristapsk approved a pull request: "Update Node window title with the chain type"
(https://github.com/bitcoin-core/gui/pull/758#pullrequestreview-1797448565)
ACK 9d37886a3b6ce24f4a4a05193eb0d071655a8457

🤔 murchandamus reviewed a pull request: "wallet: track mempool conflicts with wallet transactions"
(https://github.com/bitcoin/bitcoin/pull/27307#pullrequestreview-1797438722)
ACK 22778ac98f1bcb695de8ccf0b02fe9ebe39b01aa

💬 murchandamus commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1437201563)
In "wallet: use CWalletTx member functions to determine tx state" (80ba1d97f179121736d702b2d54bd3938cb57d96):

Did you mean "An output is considered spent…"?

💬 dimitaracev commented on pull request "wallet: add meaningful error message and fix test":
(https://github.com/bitcoin/bitcoin/pull/29143#issuecomment-1870566016)
> Also, an assertion crash fix, is not a "test" fix. I presume this can happen in production, no?

Initially I thought that this was the expected behavior, which is why this pull request just returns a different error message so that it can be caught in the test. But as @furszy said, the issue appears to be something completely different that needs fixing.

> Do you have steps to reproduce the test failure (usually a race can be reproduced by adding a sleep in the right place in the C++ cod
...

🤔 murchandamus reviewed a pull request: "wallet, rpc: add anti-fee-sniping to `send` and `sendall`"
(https://github.com/bitcoin/bitcoin/pull/28944#pullrequestreview-1797462130)
Concept ACK and almost crACK d76f88051524971def5137a07ce2940dce1c33a9 except that there seems to be a type mix-up:

```
wallet/rpc/spend.cpp:1299:20: error: no matching function for call to 'FinishTransaction'
return FinishTransaction(pwallet, options, CMutableTransaction(*txr.tx));
^~~~~~~~~~~~~~~~~
wallet/rpc/spend.cpp:79:17: note: candidate function not viable: expects an lvalue for 3rd argument
static UniValue FinishTransaction(const std::shared_ptr<CW
...

🤔 murchandamus reviewed a pull request: "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs"
(https://github.com/bitcoin/bitcoin/pull/28979#pullrequestreview-1797464732)
Concept ACK, the ancestor aware funding could use a test

💬 murchandamus commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1437219363)
In "rpc, test: test sendall spends unconfirmed change and unconfirmed inputs when specified" 69408120a64316624b9ec379771dbac142dc6646, perhaps you could confirm here that the wallet is now empty:

```
assert_equal(self.wallet.getbalance(), 0)
```

💬 murchandamus commented on pull request "wallet, rpc: document and update `sendall` behavior around unconfirmed inputs":
(https://github.com/bitcoin/bitcoin/pull/28979#discussion_r1437221305)
This could be tested by first spending an input whose parent transaction has a higher feerate than what we aim for, then an input of the same type with the same amount whose parent transaction has a lower feerate and showing that the amount sent to the receiver is amended correspondingly.

💬 ishaanam commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#issuecomment-1870602431)
Fixed silent merge conflict.

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1437240468)
Fixed

🤔 murchandamus reviewed a pull request: "wallet, rpc: add anti-fee-sniping to `send` and `sendall`"
(https://github.com/bitcoin/bitcoin/pull/28944#pullrequestreview-1797513405)
ACK c8de459de6d22e37164583f078747facaeae69c6

💬 murchandamus commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#issuecomment-1870633370)
ACK https://github.com/bitcoin/bitcoin/pull/27307/commits/d4259cdaff2128c25e877d6a64e6ce15ecb9beca, only documentation change

💬 mzumsande commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1437279158)
Ok, I clarified the comments added in this PR.
I think that a single connection / reconnection attempt doesn't waste a meaningful amount of resources of either side, so I'm not sure if there is an actual problem here. Also, often one might not want the other side to know that you evicted them because they are banned (after all you banned them for a reason) - that might just help them evade the ban.

💬 mzumsande commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1870670097)
[a30a1d9 ](https://github.com/bitcoin/bitcoin/commit/a30a1d9318d08ddd1007d7fe7c19e4175faa8b64)to [fb5bfed](https://github.com/bitcoin/bitcoin/commit/fb5bfed26a564014b83ccfc96ff00b630930fc61): small doc-only update.