💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1446186725)
Thanks, I did some hard staring at this while-loop. I restructured it to use a `break` as you suggested, and was able to reduce the loop’s condition to:

```diff
- while (!should_shift && next_utxo > 0
- && (curr_selection.empty() || curr_selection.back() != next_utxo - 1)
- && utxo_pool[next_utxo - 1].GetSelectionAmount() == utxo_pool[next_utxo].GetSelectionAmount()
+ while (utxo_pool[next_utxo - 1].GetSelectionAmount() == utxo_p
...

💬 jamesob commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1446512814)
Should probably have some belt-and-suspenders check that we're 100% not running on mainnet, maybe based on the contents of `params` and whatever applicable global.

🤔 sr-gi reviewed a pull request: "p2p: Fill reconciliation sets (Erlay)"
(https://github.com/bitcoin/bitcoin/pull/28765#pullrequestreview-1811832956)
Some additional comments after the recent optimizations + caching

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1446503850)
nit: `targes` -> `targets`

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1446517082)
nit: I'd be nice to define this as a constant

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1446518794)
I don't think this is right(?), this logic is never triggered.

You are checking whether the size of `tx_fanout_targes_cache_order` is over the limit, but you are only adding elements to the queue within that same `if` context, meaning that the queue is always empty.

👍 alfonsoromanz approved a pull request: "doc: Add example of mixing private and public keys in descriptors"
(https://github.com/bitcoin/bitcoin/pull/28373#pullrequestreview-1811870092)
ACK

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1883691930)
Updated https://github.com/bitcoin/bitcoin/commit/909c6bb1dbc6f60c5ad3200d83f3fce98f4c3706 -> https://github.com/bitcoin/bitcoin/commit/c856a1093009c6098c7c9fed5cc3f03fd0c0cf5b ([compare](https://github.com/josibake/bitcoin/compare/909c6bb1dbc6f60c5ad3200d83f3fce98f4c3706..c856a1093009c6098c7c9fed5cc3f03fd0c0cf5b))

* Moved `CTxDestination`, `Amount` into loop
* Refactored `InterpretSubtractFeeFromAmountInstructions` to infer parsing logic based on the instruction type (int, bool, str)

Tha
...

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1446538974)
Fixed

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1446539102)
Done

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1446539347)
Done

💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1446539625)
Fixed

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1446545360)
done

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1446545395)
left a comment for future work

👍 alfonsoromanz approved a pull request: "getrawtransaction implementation"
(https://github.com/bitcoin-core/gui/pull/777#pullrequestreview-1811945070)
Tested ACK https://github.com/bitcoin-core/gui/pull/777/commits/9659890f7c9223d85dda3d066c2e3de42cabe7cf


💬 maflcko commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1446601858)
```
node0 stderr node/blockstorage.cpp:1070:15: runtime error: unsigned integer overflow: 0 - 8 cannot be represented in type 'unsigned int'
#0 0x561baaf1fb61 in node::BlockManager::ReadRawBlockFromDisk(std::vector<unsigned char, std::allocator<unsigned char>>&, FlatFilePos const&) const src/node/blockstorage.cpp:1070:15
#1 0x561bab1a45bf in GetRawBlockChecked(node::BlockManager&, CBlockIndex const&) src/rpc/blockchain.cpp:610:19
#2 0x561bab1a45bf in getblock()::$_0::operator()(
...

💬 maflcko commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1446602245)
https://cirrus-ci.com/task/6333299799359488?logs=ci#L4284

💬 maflcko commented on pull request "Detect and ignore transactions that were CPFP'd in the fee estimator":
(https://github.com/bitcoin/bitcoin/pull/25380#issuecomment-1883778620)
Needs rebase, if still relevant

💬 jonatack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1446607171)
fb5bfed26a56401 This will cause `-netinfo` to break when calling it on a node that is running pre-v26 bitcoind, as `getpeerinfo` doesn't yet return a "transport_protocol_type" field. It just needs an `IsNull()` check, as done in the next line after and for a few other fields.

📝 jonatack opened a pull request: "Fix -netinfo backward compat with getpeerinfo pre-v26"
(https://github.com/bitcoin/bitcoin/pull/29212)
Commit fb5bfed26a564014b83ccfc96ff00b630930fc61 in #29058 will cause `-netinfo` to break when calling it on a node that is running pre-v26 bitcoind, as `getpeerinfo` doesn't yet return a "transport_protocol_type" field.

Fix this by adding an `IsNull()` check, as already done for other fields, and also optionally in the same commit:

a) avoid checking for the full string "detecting", and instead do the cheaper check for the most frequent case of the string starting with "v"

b) drop displa
...