💬 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
(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
(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.
(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
...
(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
...
(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.
(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
(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.
(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.
(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?
(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
(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!)
(https://github.com/bitcoin/bitcoin/pull/29212#issuecomment-1883825948)
Re-pushed per @kristapsk's feedback (thanks!)
💬 sipa commented on pull request "Fix -netinfo backward compat with getpeerinfo pre-v26":
(https://github.com/bitcoin/bitcoin/pull/29212#issuecomment-1883827869)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29212#issuecomment-1883827869)
Concept ACK
📝 mzumsande opened a pull request: "doc, test: test and explain service flag handling"
(https://github.com/bitcoin/bitcoin/pull/29213)
Service flags received on the peer-to-peer network are handled differently, depending on how we receive them.
If received directly from an outbound peer the flags belong to, they replace existing flags.
If received via gossip relay (so that anyone could send them), new flags are added, but existing ones but cannot be overwritten.
Document that and add test coverage for it.
(https://github.com/bitcoin/bitcoin/pull/29213)
Service flags received on the peer-to-peer network are handled differently, depending on how we receive them.
If received directly from an outbound peer the flags belong to, they replace existing flags.
If received via gossip relay (so that anyone could send them), new flags are added, but existing ones but cannot be overwritten.
Document that and add test coverage for it.
👍 kristapsk approved a pull request: "Fix -netinfo backward compat with getpeerinfo pre-v26"
(https://github.com/bitcoin/bitcoin/pull/29212#pullrequestreview-1812079109)
ACK 5fa74609b833643334dfb5519f2023119984267b
(https://github.com/bitcoin/bitcoin/pull/29212#pullrequestreview-1812079109)
ACK 5fa74609b833643334dfb5519f2023119984267b
💬 jonatack commented on pull request "doc, test: test and explain service flag handling":
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1883866390)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29213#issuecomment-1883866390)
Concept ACK
💬 sipa commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-1883870883)
@instagibbs See https://github.com/sipa/bitcoin/commits/pr28984 for a commit that works exactly for all `FeeFrac` objects. It uses C++20 three-way comparisons to also reduce the line count by 25%.
(https://github.com/bitcoin/bitcoin/pull/28984#issuecomment-1883870883)
@instagibbs See https://github.com/sipa/bitcoin/commits/pr28984 for a commit that works exactly for all `FeeFrac` objects. It uses C++20 three-way comparisons to also reduce the line count by 25%.
🤔 mzumsande reviewed a pull request: "Fix -netinfo backward compat with getpeerinfo pre-v26"
(https://github.com/bitcoin/bitcoin/pull/29212#pullrequestreview-1812115477)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29212#pullrequestreview-1812115477)
Concept ACK
💬 mzumsande commented on pull request "Fix -netinfo backward compat with getpeerinfo pre-v26":
(https://github.com/bitcoin/bitcoin/pull/29212#discussion_r1446679605)
I think that if we don't write the "v", we can make the width of the added column 1 instead of 2.
(https://github.com/bitcoin/bitcoin/pull/29212#discussion_r1446679605)
I think that if we don't write the "v", we can make the width of the added column 1 instead of 2.
💬 jonatack commented on pull request "Fix -netinfo backward compat with getpeerinfo pre-v26":
(https://github.com/bitcoin/bitcoin/pull/29212#discussion_r1446686612)
Yes, I began with that, but it looked better to my eye when I stayed with the width you set.
(https://github.com/bitcoin/bitcoin/pull/29212#discussion_r1446686612)
Yes, I began with that, but it looked better to my eye when I stayed with the width you set.