💬 sipa commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#issuecomment-1638783636)
@MarcoFalke Thanks for the comments; I think I'm going to create a separate PR with some further code refactoring related to this as well, outside of the critical path of #25361.
(https://github.com/bitcoin/bitcoin/pull/27993#issuecomment-1638783636)
@MarcoFalke Thanks for the comments; I think I'm going to create a separate PR with some further code refactoring related to this as well, outside of the critical path of #25361.
💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265859575)
Very nice, done.
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265859575)
Very nice, done.
💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265859783)
Much better, thank you!
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265859783)
Much better, thank you!
💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265860411)
I made the declaration on one line for the smaller diff, and I don't mind reading the parameter list left-to-right. But sure, kept it multi-line.
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265860411)
I made the declaration on one line for the smaller diff, and I don't mind reading the parameter list left-to-right. But sure, kept it multi-line.
🤔 mzumsande reviewed a pull request: "[NO MERGE] BIP331 Ancestor Package Relay"
(https://github.com/bitcoin/bitcoin/pull/27742#pullrequestreview-1531801228)
Some initial comments on part 2 (Negotiate Package Relay, Request Ancestor Package Info For Orphans) - will move on to part 3 soon.
(https://github.com/bitcoin/bitcoin/pull/27742#pullrequestreview-1531801228)
Some initial comments on part 2 (Negotiate Package Relay, Request Ancestor Package Info For Orphans) - will move on to part 3 soon.
💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1264747557)
nit: `wtxids`, this one is not a copy.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1264747557)
nit: `wtxids`, this one is not a copy.
💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265523786)
3494f96ebacf53cbd5fd09f6421d9dabec87361e:
Do we need to adjust the logic in `ProcessOrphanTx()` too, by inserting there into `m_recent_rejects_reconsiderable` instead of `m_recent_rejects` if the reason was low-fee? It currently adds the wtxid of a reconsidered orphan to `m_recent_rejects` if it fails for policy/fee-related reasons. But if that resolved orphan is itself one of the parent of another orphan we wouldn't consider that (possibly high-fee) child because a parent was rejected.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265523786)
3494f96ebacf53cbd5fd09f6421d9dabec87361e:
Do we need to adjust the logic in `ProcessOrphanTx()` too, by inserting there into `m_recent_rejects_reconsiderable` instead of `m_recent_rejects` if the reason was low-fee? It currently adds the wtxid of a reconsidered orphan to `m_recent_rejects` if it fails for policy/fee-related reasons. But if that resolved orphan is itself one of the parent of another orphan we wouldn't consider that (possibly high-fee) child because a parent was rejected.
💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265739284)
The approach to add a wtxid disguised as a txid to distinguish between legacy orphan processing and package relay seems a bit like a hack to me.
I'm not strictly against it, but I guess I don't completely understand yet why it's necessary - the information whether a peer supports package relay does not change, so why can't we just always use `GenTxid::Wtxid(wtxid)` and check again in `GetOrphanRequests()` and elsewhere whether we do package relay with the peer instead of checking whether it's
...
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265739284)
The approach to add a wtxid disguised as a txid to distinguish between legacy orphan processing and package relay seems a bit like a hack to me.
I'm not strictly against it, but I guess I don't completely understand yet why it's necessary - the information whether a peer supports package relay does not change, so why can't we just always use `GenTxid::Wtxid(wtxid)` and check again in `GetOrphanRequests()` and elsewhere whether we do package relay with the peer instead of checking whether it's
...
💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1264748895)
3494f96ebacf53cbd5fd09f6421d9dabec87361e:
should update this line too because fee is no longer included in it
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1264748895)
3494f96ebacf53cbd5fd09f6421d9dabec87361e:
should update this line too because fee is no longer included in it
💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265806240)
It's an unsolicited `NOTFOUND` of type `MSG_ANCPKGINFO`, would be good to distinguish between the inv type and the actual `ANCPKGINFO` message.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265806240)
It's an unsolicited `NOTFOUND` of type `MSG_ANCPKGINFO`, would be good to distinguish between the inv type and the actual `ANCPKGINFO` message.
💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265811166)
`PkgInfoAllowed()` calling `ReceivedResponse()` as a side effect is surprising to me. Wouldn't it be better if the caller would also have to explicitly call `ForgetPkgInfo()` if they want to forget about the package? This is also not mentioned in its doc.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265811166)
`PkgInfoAllowed()` calling `ReceivedResponse()` as a side effect is surprising to me. Wouldn't it be better if the caller would also have to explicitly call `ForgetPkgInfo()` if they want to forget about the package? This is also not mentioned in its doc.
💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265856298)
We should also disable sending "sendpackages" for outgoing block-relay-only connections and probably also feelers, would suggest to just use `RejectIncomingTxs()`.
Also, I think this should be moved up and combined with the `m_enable_package_relay` check two lines above, because I see no reason to call `m_txpackagetracker->ReceivedVersion()` in these scenarios.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265856298)
We should also disable sending "sendpackages" for outgoing block-relay-only connections and probably also feelers, would suggest to just use `RejectIncomingTxs()`.
Also, I think this should be moved up and combined with the `m_enable_package_relay` check two lines above, because I see no reason to call `m_txpackagetracker->ReceivedVersion()` in these scenarios.
💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265838609)
I think that the splitting into commits is not ideal here: In ae2664793b1d0d86bfa32c3e635d83b1ee083e60, we add logic to return the wtxid, expecting it to be requested by `MSG_ANCPKGINFO`. But the net_processing code to actually do this is only added in dd6fb139b8887f3400a65dc0d4c4ee54cf53d0bc, so that at ae2664793b1d0d86bfa32c3e635d83b1ee083e60, we'd send out a `MSG_TX` getdata, thus re-requesting a TX we already know. These two things should go together into one commit.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265838609)
I think that the splitting into commits is not ideal here: In ae2664793b1d0d86bfa32c3e635d83b1ee083e60, we add logic to return the wtxid, expecting it to be requested by `MSG_ANCPKGINFO`. But the net_processing code to actually do this is only added in dd6fb139b8887f3400a65dc0d4c4ee54cf53d0bc, so that at ae2664793b1d0d86bfa32c3e635d83b1ee083e60, we'd send out a `MSG_TX` getdata, thus re-requesting a TX we already know. These two things should go together into one commit.
💬 instagibbs commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265868269)
It makes it very difficult to read, imo, and lilkely introduces bugs going forward, if not now.
Note @mzumsande that this logic lives in the orphan relay PR IIRC
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265868269)
It makes it very difficult to read, imo, and lilkely introduces bugs going forward, if not now.
Note @mzumsande that this logic lives in the orphan relay PR IIRC
💬 jonatack commented on pull request "net, refactor: remove unneeded exports, use helpers over low-level code, use optional":
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265869787)
Dropped the commit and instead added an `IsPrivacyNet()` helper for `GetLocal()` only, where it cleans things up. When we'll likely need to add `IsCJDNS()` to it later on, it will be simple to do and will keep `GetLocal()` easy to read.
(https://github.com/bitcoin/bitcoin/pull/28078#discussion_r1265869787)
Dropped the commit and instead added an `IsPrivacyNet()` helper for `GetLocal()` only, where it cleans things up. When we'll likely need to add `IsCJDNS()` to it later on, it will be simple to do and will keep `GetLocal()` easy to read.
💬 sipa commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1638845486)
Concept/approach ACK.
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1638845486)
Concept/approach ACK.
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1265886880)
I'm looking into this, the [spec](https://www.jsonrpc.org/specification#notification) repeats no response to notifications but it doesn't indicate whether an HTTP OK is reasonable or whether the request should be completely ignored. I'm having some trouble getting libevent to completely cancel a request without sending anything back (and still letting bitcoind shutdown without hanging!) so maybe a 200 OK is best
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1265886880)
I'm looking into this, the [spec](https://www.jsonrpc.org/specification#notification) repeats no response to notifications but it doesn't indicate whether an HTTP OK is reasonable or whether the request should be completely ignored. I'm having some trouble getting libevent to completely cancel a request without sending anything back (and still letting bitcoind shutdown without hanging!) so maybe a 200 OK is best
💬 Empact commented on pull request "validation: use noexcept instead of deprecated throw()":
(https://github.com/bitcoin/bitcoin/pull/28090#issuecomment-1638877379)
utACK https://github.com/bitcoin/bitcoin/commit/047daad4f59942488163c6be8516a69291646294
(https://github.com/bitcoin/bitcoin/pull/28090#issuecomment-1638877379)
utACK https://github.com/bitcoin/bitcoin/commit/047daad4f59942488163c6be8516a69291646294
💬 Empact commented on pull request "Enable -Wstring-concatenation and -Wstring-conversion on clang builds":
(https://github.com/bitcoin/bitcoin/pull/26288#issuecomment-1638888808)
Rebased, converted assignments to initialization, and moved to `Assert` where possible. I could not apply `Assert(false)` in interpreter.cpp due to errors, but https://github.com/bitcoin/bitcoin/pull/26504 could be applied here once merged.
(https://github.com/bitcoin/bitcoin/pull/26288#issuecomment-1638888808)
Rebased, converted assignments to initialization, and moved to `Assert` where possible. I could not apply `Assert(false)` in interpreter.cpp due to errors, but https://github.com/bitcoin/bitcoin/pull/26504 could be applied here once merged.
💬 mzumsande commented on pull request "[NO MERGE] BIP331 Ancestor Package Relay":
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265915386)
right - I'll copy the comment so that we can discuss there - this one can be resolved.
(https://github.com/bitcoin/bitcoin/pull/27742#discussion_r1265915386)
right - I'll copy the comment so that we can discuss there - this one can be resolved.