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

💬 jonatack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1883801587)
Reviewed https://github.com/bitcoin/bitcoin/commit/fb5bfed26a564014b83ccfc96ff00b630930fc61; WIP review of the other two commits.

💬 maflcko commented on pull request "test: Minor fix in test - locale in terminal":
(https://github.com/bitcoin/bitcoin/pull/28286#discussion_r1446624666)
lgtm, but if the file exists, this will also fail.

Might be better to use a pure command that does not depend on the env vars and filesystem state

💬 kristapsk commented on pull request "Fix -netinfo backward compat with getpeerinfo pre-v26":
(https://github.com/bitcoin/bitcoin/pull/29212#discussion_r1446625770)
Can't we just default to `"v1"` instead of `""` in case of null? Pre-v26 doesn't support v2 transport, it will always be v1.

💬 jonatack commented on pull request "Fix -netinfo backward compat with getpeerinfo pre-v26":
(https://github.com/bitcoin/bitcoin/pull/29212#discussion_r1446627857)
The peer could be v2, so we shouldn't display v1 or v2 until its transport protocol version is known.

💬 jonatack commented on pull request "Fix -netinfo backward compat with getpeerinfo pre-v26":
(https://github.com/bitcoin/bitcoin/pull/29212#discussion_r1446628773)
Perhaps we shouldn't display that column if getpeerinfo doesn't support it. Like for asmap?

💬 jonatack commented on pull request "Fix -netinfo backward compat with getpeerinfo pre-v26":
(https://github.com/bitcoin/bitcoin/pull/29212#discussion_r1446631011)
> Can't we just default to `"v1"` instead of `""` in case of null? Pre-v26 doesn't support v2 transport, it will always be v1.

Done

💬 jonatack commented on pull request "Fix -netinfo backward compat with getpeerinfo pre-v26":
(https://github.com/bitcoin/bitcoin/pull/29212#issuecomment-1883825948)
Re-pushed per @kristapsk's feedback (thanks!)