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

💬 jonatack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1446610917)
> @jonatack do you have an opinion?

Would suggest display nothing during peer setup, like for the -netinfo mping and ping columns (`*` already has a separate meaning in -netinfo, and `?` might look like there is a bug)

Also on this line would suggest to avoid checking for the full string "detecting", and instead do the cheaper check for the most frequent case of the string starting with "v".

Finally, would propose to drop displaying the "v" prefix in all the rows, as it doesn't add usef
...