💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901631250)
Done in 6accee18442f79fd2f2796027371a70e3757d825.
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1901631250)
Done in 6accee18442f79fd2f2796027371a70e3757d825.
💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2568971908)
Thanks for the review @ryanofsky. I took your suggestion to split `CheckProofOfWorkImpl`. Can you add the Consensus label to this PR?
@tdb3 I took your two patches and one suggestion
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2568971908)
Thanks for the review @ryanofsky. I took your suggestion to split `CheckProofOfWorkImpl`. Can you add the Consensus label to this PR?
@tdb3 I took your two patches and one suggestion
👍 hebasto approved a pull request: "ci: Enable DEBUG=1 for one GCC-12 build to catch 117966 regressions"
(https://github.com/bitcoin/bitcoin/pull/31522#pullrequestreview-2528826222)
ACK fa1d84702bf4c14d55c51b9ab4cd30cdbbc5ad44, tested locally by reverting https://github.com/bitcoin/bitcoin/commit/fa9e0489f57968945d54ef56b275f51540f3e5e4 back and observing a compiler error.
(https://github.com/bitcoin/bitcoin/pull/31522#pullrequestreview-2528826222)
ACK fa1d84702bf4c14d55c51b9ab4cd30cdbbc5ad44, tested locally by reverting https://github.com/bitcoin/bitcoin/commit/fa9e0489f57968945d54ef56b275f51540f3e5e4 back and observing a compiler error.
🤔 l0rinc reviewed a pull request: "doc: Clarify comments about endianness after #30526"
(https://github.com/bitcoin/bitcoin/pull/31596#pullrequestreview-2528790297)
Thanks for improving these!
Since we can’t usually change the behavior, it’s crucial to share tribal knowledge.
I have a few suggestions for reducing duplication, simplifying sentence structure, and applying these changes to similar places.
However, I understand if you prefer to limit the scope
(https://github.com/bitcoin/bitcoin/pull/31596#pullrequestreview-2528790297)
Thanks for improving these!
Since we can’t usually change the behavior, it’s crucial to share tribal knowledge.
I have a few suggestions for reducing duplication, simplifying sentence structure, and applying these changes to similar places.
However, I understand if you prefer to limit the scope
💬 l0rinc commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901633014)
I had to read this several times (didn't understand what a "least-significant-first base" is at first), consider the following:
```suggestion
/** Integer with 32-bit digits, least-significant first. */
```
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901633014)
I had to read this several times (didn't understand what a "least-significant-first base" is at first), consider the following:
```suggestion
/** Integer with 32-bit digits, least-significant first. */
```
💬 l0rinc commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901648055)
Could we be more compact here?
Personally I'm not a fan of long documentation - though the reason usually is that if we need to document something heavily it's a sign of poorly written code - not something we can really change here.
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901648055)
Could we be more compact here?
Personally I'm not a fan of long documentation - though the reason usually is that if we need to document something heavily it's a sign of poorly written code - not something we can really change here.
💬 l0rinc commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901655760)
documenting `txid` as "it's the txid hash" is a bit redundant and confusing.
In https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L135 (and a few other places) the description is `"The transaction hash in hex"`.
Could we unify the rest as well to something simple like
```suggestion
{RPCResult::Type::STR_HEX, "txid", "transaction id hash (without witness) shown in byte-reversed hex"},
```
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901655760)
documenting `txid` as "it's the txid hash" is a bit redundant and confusing.
In https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L135 (and a few other places) the description is `"The transaction hash in hex"`.
Could we unify the rest as well to something simple like
```suggestion
{RPCResult::Type::STR_HEX, "txid", "transaction id hash (without witness) shown in byte-reversed hex"},
```
💬 l0rinc commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901612593)
```suggestion
* This means, for example, that ArithToUint256(num).GetHex() can be used to
* display an arith_uint256 num value as a number, because
```
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901612593)
```suggestion
* This means, for example, that ArithToUint256(num).GetHex() can be used to
* display an arith_uint256 num value as a number, because
```
💬 l0rinc commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901643256)
According to Wiki `little endian` is preferred with a dash: https://en.wikipedia.org/wiki/Endianness
But the part I find confusing is rather expressing 3 concepts in a single sentence - can we break it up a bit?
```suggestion
/** Get the hash of the wtxids of these transactions, treating each wtxid hash
* as a little-endian number and sorting them in ascending numeric order.
* This is equivalent to reversing the bytes of each wtxid hash and sorting them lexicographically.
*/
```
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901643256)
According to Wiki `little endian` is preferred with a dash: https://en.wikipedia.org/wiki/Endianness
But the part I find confusing is rather expressing 3 concepts in a single sentence - can we break it up a bit?
```suggestion
/** Get the hash of the wtxids of these transactions, treating each wtxid hash
* as a little-endian number and sorting them in ascending numeric order.
* This is equivalent to reversing the bytes of each wtxid hash and sorting them lexicographically.
*/
```
💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669070)
That makes sense, added an explicit check above the `waitTipChanged()` call.
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669070)
That makes sense, added an explicit check above the `waitTipChanged()` call.
💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669119)
I dropped this comment.
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669119)
I dropped this comment.
💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669167)
Ok, I dropped them for the commit and PR description and updated the code comment.
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669167)
Ok, I dropped them for the commit and PR description and updated the code comment.
💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669229)
Renamed.
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669229)
Renamed.
💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669273)
A failing `BOOST_REQUIRE` produces an easier to debug failure than dereferencing nullptr, so it's always a good one to add.
I updated the test to do this after every `miner->createNewBlock()` call.
Similarly `BOOST_REQUIRE` stops the test immedidately, so I replaced a few `BOOST_CHECK` calls with it.
(https://github.com/bitcoin/bitcoin/pull/31581#discussion_r1901669273)
A failing `BOOST_REQUIRE` produces an easier to debug failure than dereferencing nullptr, so it's always a good one to add.
I updated the test to do this after every `miner->createNewBlock()` call.
Similarly `BOOST_REQUIRE` stops the test immedidately, so I replaced a few `BOOST_CHECK` calls with it.
💬 Sjors commented on pull request "test: have miner_tests use Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31581#issuecomment-2569034227)
Rebased and addressed feedback.
(https://github.com/bitcoin/bitcoin/pull/31581#issuecomment-2569034227)
Rebased and addressed feedback.
💬 Sjors commented on pull request "doc: Clarify comments about endianness after #30526":
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901672336)
I think it's a useful comment.
(https://github.com/bitcoin/bitcoin/pull/31596#discussion_r1901672336)
I think it's a useful comment.
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2569056045)
> Could you clarify the effect of setting it to 4000 in light of the recent findings?
It's still not worth the risk imo. Making the default 4000 instead of 8000 provides 0.1% extra fee revenue for miners who don't read the release notes. But there could be a miner out there losing 100% of the block revenue (subsidy + 100% of fees) because they were doing something custom that you didn't detect in the analysis.
In the example of Ocean it _seems_ that their custom `-blockmaxweight` is low en
...
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2569056045)
> Could you clarify the effect of setting it to 4000 in light of the recent findings?
It's still not worth the risk imo. Making the default 4000 instead of 8000 provides 0.1% extra fee revenue for miners who don't read the release notes. But there could be a miner out there losing 100% of the block revenue (subsidy + 100% of fees) because they were doing something custom that you didn't detect in the analysis.
In the example of Ocean it _seems_ that their custom `-blockmaxweight` is low en
...
💬 maflcko commented on pull request "refactor: Allow std::byte in Read(LE/BE)":
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1901686821)
> I don't think it would be possible to write a clang-tidy plugin to check for these cases
I haven't tried it, so I am not sure if it is possible. However, looking at the AST, it should be trivial to detect those functions. Matching any `FunctionTemplateDecl` whose `FunctionDecl` does not have any `TemplateTypeParm` in the AST should be sufficient to find those that can be written without `template<...>`.
Example: https://godbolt.org/z/4rre4rPT6
(https://github.com/bitcoin/bitcoin/pull/31524#discussion_r1901686821)
> I don't think it would be possible to write a clang-tidy plugin to check for these cases
I haven't tried it, so I am not sure if it is possible. However, looking at the AST, it should be trivial to detect those functions. Matching any `FunctionTemplateDecl` whose `FunctionDecl` does not have any `TemplateTypeParm` in the AST should be sufficient to find those that can be written without `template<...>`.
Example: https://godbolt.org/z/4rre4rPT6
💬 stickies-v commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1901720687)
> The chainman reset may trigger validation interface notifications
Ah, right, `ChainStateFlushed` would be called. Yeah, I think with that knowledge doing it afterwards makes more sense. Also, callbacks are processed regardless of `SyncWithValidationInterfaceQueue()` being called, so I suppose this doesn't really change anything wrt callbacks assuming `m_node.chainman` exists.
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1901720687)
> The chainman reset may trigger validation interface notifications
Ah, right, `ChainStateFlushed` would be called. Yeah, I think with that knowledge doing it afterwards makes more sense. Also, callbacks are processed regardless of `SyncWithValidationInterfaceQueue()` being called, so I suppose this doesn't really change anything wrt callbacks assuming `m_node.chainman` exists.
🚀 glozow merged a pull request: "include verbose "reject-details" field in testmempoolaccept response"
(https://github.com/bitcoin/bitcoin/pull/28121)
(https://github.com/bitcoin/bitcoin/pull/28121)