💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2286722016)
I introduced `using NoiseHash = std::array<uint8_t, HASHLEN>;` for improved readability.
Although all tests passed, I noticed while testing #29432 that SRI hangs up after the handshake. Turns out I broke the `HKDF2` implementation, which the test didn't catch because there's no hardcoded test vector. That's fixed now.
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2286722016)
I introduced `using NoiseHash = std::array<uint8_t, HASHLEN>;` for improved readability.
Although all tests passed, I noticed while testing #29432 that SRI hangs up after the handshake. Turns out I broke the `HKDF2` implementation, which the test didn't catch because there's no hardcoded test vector. That's fixed now.
💬 martinsaposnic commented on issue "doc: deduplicate list of chain/network strings in RPC/parameter help texts":
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2286723952)
new PR with possible solution: https://github.com/bitcoin/bitcoin/pull/30648
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2286723952)
new PR with possible solution: https://github.com/bitcoin/bitcoin/pull/30648
👍 ryanofsky approved a pull request: "refactor: Add consteval ArrayFromBytes()"
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2236080683)
Did not review in detail yet but light code review ACK d6d7a0b5221935518d2797aec7abc5c9632cbf68. All changes seem like what I would expect.
I think I would suggest changing title of PR to "refactor: Replace ParseHex with consteval ArrayFromHex" so it mentions ParseHex and it is more obvious how this affects existing code.
(https://github.com/bitcoin/bitcoin/pull/30377#pullrequestreview-2236080683)
Did not review in detail yet but light code review ACK d6d7a0b5221935518d2797aec7abc5c9632cbf68. All changes seem like what I would expect.
I think I would suggest changing title of PR to "refactor: Replace ParseHex with consteval ArrayFromHex" so it mentions ParseHex and it is more obvious how this affects existing code.
💬 ryanofsky commented on pull request "refactor: Add consteval ArrayFromBytes()":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1715636099)
In commit "refactor: Add consteval ArrayFromHex()" (00dc16bc29124dfbaf9d47c5411e4ebcf95e30b5)
This warning seems unnecessarily scary and maybe not true. I don't think the function itself would use any stack space with NRVO, and callers could be calling this to initialize an array on the heap not the stack. Would suggest deleting this comment, because it seems like a warning about std::array in general, and I don't think there is anything that really distinguishes this function from other func
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1715636099)
In commit "refactor: Add consteval ArrayFromHex()" (00dc16bc29124dfbaf9d47c5411e4ebcf95e30b5)
This warning seems unnecessarily scary and maybe not true. I don't think the function itself would use any stack space with NRVO, and callers could be calling this to initialize an array on the heap not the stack. Would suggest deleting this comment, because it seems like a warning about std::array in general, and I don't think there is anything that really distinguishes this function from other func
...
💬 ryanofsky commented on pull request "refactor: Add consteval ArrayFromBytes()":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1715621085)
In commit "refactor: Add consteval ArrayFromHex()" (00dc16bc29124dfbaf9d47c5411e4ebcf95e30b5)
Is this necessary? I'd be concerned adding such a generic top-level operator might cause this to get called in places we are not expecting, and maybe lead to hard to explain behavior or bugs. Would be good to at least have a comment explaining what this is intended to be used for.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1715621085)
In commit "refactor: Add consteval ArrayFromHex()" (00dc16bc29124dfbaf9d47c5411e4ebcf95e30b5)
Is this necessary? I'd be concerned adding such a generic top-level operator might cause this to get called in places we are not expecting, and maybe lead to hard to explain behavior or bugs. Would be good to at least have a comment explaining what this is intended to be used for.
💬 ryanofsky commented on pull request "refactor: Add consteval ArrayFromBytes()":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1715642251)
In commit "refactor: Make code more tolerant of constexpr std::arrays" (05c16ae8649ee3b6bbcdbdf0541a65ccf4477537)
What is this referring to?
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1715642251)
In commit "refactor: Make code more tolerant of constexpr std::arrays" (05c16ae8649ee3b6bbcdbdf0541a65ccf4477537)
What is this referring to?
💬 achow101 commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2286740593)
Update the BIP first?
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2286740593)
Update the BIP first?
💬 instagibbs commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2286755243)
I'm confused why is this touching ancient stuff unrelated to testnet4?
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2286755243)
I'm confused why is this touching ancient stuff unrelated to testnet4?
💬 ryanofsky commented on pull request "log: expand BCLog::LogFlags (categories) to 64 bits":
(https://github.com/bitcoin/bitcoin/pull/26619#issuecomment-2286760992)
Code review ACK f64b861c0940ca90869303a9754ebe5bf40bb0c7, just added uint64_t typedef since last review.
> As a test, I defined `CategoryMask` to be `uint32_t`, but the compile failed because of the `ULL` suffix on all those constants (width exceeds 32 bits). So I'm trying to work around that by defining a `mask_bit` constant. (Should it have a name like `MaskBit`? It's only used within the `enum` definition.) I think it makes the code easy to read. If CI fails, I can change them back to `ULL
...
(https://github.com/bitcoin/bitcoin/pull/26619#issuecomment-2286760992)
Code review ACK f64b861c0940ca90869303a9754ebe5bf40bb0c7, just added uint64_t typedef since last review.
> As a test, I defined `CategoryMask` to be `uint32_t`, but the compile failed because of the `ULL` suffix on all those constants (width exceeds 32 bits). So I'm trying to work around that by defining a `mask_bit` constant. (Should it have a name like `MaskBit`? It's only used within the `enum` definition.) I think it makes the code easy to read. If CI fails, I can change them back to `ULL
...
👋 epiccurious's pull request is ready for review: "feat(build): improve dependency error reporting in autogen.sh"
(https://github.com/bitcoin/bitcoin/pull/30646)
(https://github.com/bitcoin/bitcoin/pull/30646)
💬 fjahr commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2286765322)
Should be setting `TIMESTAMP_WINDOW` to 600 not `MAX_FUTURE_BLOCK_TIME`.
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2286765322)
Should be setting `TIMESTAMP_WINDOW` to 600 not `MAX_FUTURE_BLOCK_TIME`.
💬 epiccurious commented on pull request "feat(build): improve dependency error reporting in autogen.sh":
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2286767473)
Taking back out of draft. PR is now the single change to the main `autogen.sh`.
(https://github.com/bitcoin/bitcoin/pull/30646#issuecomment-2286767473)
Taking back out of draft. PR is now the single change to the main `autogen.sh`.
💬 fjahr commented on pull request "Move maximum timewarp attack threshold back to 600s from 7200s":
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2286774804)
Concept ACK
I was going with a more conservative approach because the timewarp attack didn't seem highest on the wishlist of what people wanted out of Testnet4 but the argument to rather break things on testnet than later on mainnet convinced me.
(https://github.com/bitcoin/bitcoin/pull/30647#issuecomment-2286774804)
Concept ACK
I was going with a more conservative approach because the timewarp attack didn't seem highest on the wishlist of what people wanted out of Testnet4 but the argument to rather break things on testnet than later on mainnet convinced me.
💬 gmaxwell commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2286788066)
@vasild you appear to have disregarded my above message about this change is desirable. Was it unclear?
With respect to breakage-- it's not uncommon that fixing bugs requires fixing bugs in other software. Fortunately, it's usually possible to balance the issues. One way to do so is to phase in changes so that older software has an opportunity to fix itself, and once the fixed behavior is ubiquitous then the compatibility issue no longer exists. But if you over narrowly analyze the first s
...
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2286788066)
@vasild you appear to have disregarded my above message about this change is desirable. Was it unclear?
With respect to breakage-- it's not uncommon that fixing bugs requires fixing bugs in other software. Fortunately, it's usually possible to balance the issues. One way to do so is to phase in changes so that older software has an opportunity to fix itself, and once the fixed behavior is ubiquitous then the compatibility issue no longer exists. But if you over narrowly analyze the first s
...
💬 brunoerg commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1715690103)
In 4a955955ec587120fb121e5068e3bc1f80406ac5 "net_processing: handle ConnectionType::PRIVATE_BROADCAST connections": It seems `FinishBroadcast` returns a `bool`, but this value is never used.
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1715690103)
In 4a955955ec587120fb121e5068e3bc1f80406ac5 "net_processing: handle ConnectionType::PRIVATE_BROADCAST connections": It seems `FinishBroadcast` returns a `bool`, but this value is never used.
🤔 theStack reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2236038602)
Concept ACK
Lightly reviewed about half of the refactoring commits, left some non-critical stuff and comments on the way below.
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2236038602)
Concept ACK
Lightly reviewed about half of the refactoring commits, left some non-critical stuff and comments on the way below.
💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715592140)
```suggestion
// If the chain tip has changed, previously rejected transactions might now be valid, e.g. due
```
also, this comment now exists both in `PeerManagerImpl::ActiveTipChange` and `TxDownloadImpl::ActiveTipChange`, probably one can be removed
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715592140)
```suggestion
// If the chain tip has changed, previously rejected transactions might now be valid, e.g. due
```
also, this comment now exists both in `PeerManagerImpl::ActiveTipChange` and `TxDownloadImpl::ActiveTipChange`, probably one can be removed
💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715622227)
nit: strictly speaking this is not a pure refactor, as there is a change in logging behaviour (INV messages received during IBD now don't trigger the "got inv" log message any more). seems to make more sense though anyway, so feel free to ignore
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715622227)
nit: strictly speaking this is not a pure refactor, as there is a change in logging behaviour (INV messages received during IBD now don't trigger the "got inv" log message any more). seems to make more sense though anyway, so feel free to ignore
💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715630850)
small note: with this approach, `AlreadyHaveTx` is now called twice: once in the if condition, and the second time in the `AddTxAnnouncement` method (not sure if that's really a problem though)
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715630850)
small note: with this approach, `AlreadyHaveTx` is now called twice: once in the if condition, and the second time in the `AddTxAnnouncement` method (not sure if that's really a problem though)
💬 theStack commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715600184)
slightly-more-modern-nit (here and in `TxDownloadImpl::DisconnectedPeer` below):
```suggestion
if (m_peer_info.contains(nodeid)) return;
```
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1715600184)
slightly-more-modern-nit (here and in `TxDownloadImpl::DisconnectedPeer` below):
```suggestion
if (m_peer_info.contains(nodeid)) return;
```
💬 Sjors commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2286816238)
I rebased https://github.com/Sjors/bitcoin/pull/48 to take this PR and https://github.com/bitcoin/bitcoin/pull/30509 into account. Already looked at few commits, but got distracted by a bug on my end, so will continue later.
(https://github.com/bitcoin/bitcoin/pull/30510#issuecomment-2286816238)
I rebased https://github.com/Sjors/bitcoin/pull/48 to take this PR and https://github.com/bitcoin/bitcoin/pull/30509 into account. Already looked at few commits, but got distracted by a bug on my end, so will continue later.