🤔 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.
💬 mzumsande commented on pull request "Package Relay 1/3: Introduce TxPackageTracker as Orphan Resolution Module":
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1265917707)
(moved over from [27742](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 `GetOrphanRe
...
(https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1265917707)
(moved over from [27742](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 `GetOrphanRe
...
👍 jonatack approved a pull request: "I2P: also sleep after errors in Accept() & destroy the session if we get "Session was closed""
(https://github.com/bitcoin/bitcoin/pull/28077#pullrequestreview-1533755139)
ACK ffa90fceae9b0c0ca2e720fae9e54dd3d094c988
Tested behavior before/after this pull using both the I2P Java router and with i2pd.
(https://github.com/bitcoin/bitcoin/pull/28077#pullrequestreview-1533755139)
ACK ffa90fceae9b0c0ca2e720fae9e54dd3d094c988
Tested behavior before/after this pull using both the I2P Java router and with i2pd.
💬 achow101 commented on pull request "Make poly1305 support incremental computation + modernize":
(https://github.com/bitcoin/bitcoin/pull/27993#issuecomment-1638966897)
ACK 4e5c933f6a40c07d1c68115f7979b89a5b2ba580
Verified that this was consistent with poly1305-donna.
(https://github.com/bitcoin/bitcoin/pull/27993#issuecomment-1638966897)
ACK 4e5c933f6a40c07d1c68115f7979b89a5b2ba580
Verified that this was consistent with poly1305-donna.
🚀 achow101 merged a pull request: "Make poly1305 support incremental computation + modernize"
(https://github.com/bitcoin/bitcoin/pull/27993)
(https://github.com/bitcoin/bitcoin/pull/27993)