💬 ryanofsky commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1744107968)
Followup points:
- I misunderstood "the build system may assign" to mean "cmake may assign" but actually it is referring to our own [ProcessConfigurations.cmake](https://github.com/bitcoin/bitcoin/blob/b8d2f58e06439e9523c02ca1729ff20cd0ba53ca/cmake/module/ProcessConfigurations.cmake#L145) file. So I think a more accurate comment than what I suggested would be "Set CMAKE_{C,CXX}\_FLAGS_DEBUG to empty so the default options normally used for debug builds do not override MSAN options in CMAKE_{C
...
(https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1744107968)
Followup points:
- I misunderstood "the build system may assign" to mean "cmake may assign" but actually it is referring to our own [ProcessConfigurations.cmake](https://github.com/bitcoin/bitcoin/blob/b8d2f58e06439e9523c02ca1729ff20cd0ba53ca/cmake/module/ProcessConfigurations.cmake#L145) file. So I think a more accurate comment than what I suggested would be "Set CMAKE_{C,CXX}\_FLAGS_DEBUG to empty so the default options normally used for debug builds do not override MSAN options in CMAKE_{C
...
💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744124233)
Thanks for looking into it @marcofleon! If there are no operational concerns adding the test cases, I'm happy with it.
I guess if we keep this, the `uint256::FromHex()` asserts have become superfluous and could be removed. Shouldn't make a meaningful performance impact (since they're called much less often than the `FromUserHex()` asserts, so it's more of a thing to keep the test code lean.
<details>
<summary>git diff on 6fa4c92e4c</summary>
```diff
diff --git a/src/test/fuzz/hex.cpp
...
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744124233)
Thanks for looking into it @marcofleon! If there are no operational concerns adding the test cases, I'm happy with it.
I guess if we keep this, the `uint256::FromHex()` asserts have become superfluous and could be removed. Shouldn't make a meaningful performance impact (since they're called much less often than the `FromUserHex()` asserts, so it's more of a thing to keep the test code lean.
<details>
<summary>git diff on 6fa4c92e4c</summary>
```diff
diff --git a/src/test/fuzz/hex.cpp
...
💬 stickies-v commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744129399)
No I don't feel strongly about it, I just think this change is not nearly useful enough to warrant an extra 33LoC diff. Anyway, not worth arguing over, like I said it's not blocking.
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744129399)
No I don't feel strongly about it, I just think this change is not nearly useful enough to warrant an extra 33LoC diff. Anyway, not worth arguing over, like I said it's not blocking.
👍 ryanofsky approved a pull request: "kernel: Create usable static kernel library"
(https://github.com/bitcoin/bitcoin/pull/30814#pullrequestreview-2280796157)
Code review ACK 192d71e312d0eab6cc1ce061edb34d9d338f7fec.
This seems like a reasonable approach, but I wonder if a more consistent way of doing this would just treat all the libraries the same as secp, installing them to the lib/ directory and listing them in libbitcoinkernel.pc. That way the secp library would not be an outlier, the TARGET_OBJECTS calls would not be needed, and build would be lighter, with each .o object file added to one library file .a instead of two.
(https://github.com/bitcoin/bitcoin/pull/30814#pullrequestreview-2280796157)
Code review ACK 192d71e312d0eab6cc1ce061edb34d9d338f7fec.
This seems like a reasonable approach, but I wonder if a more consistent way of doing this would just treat all the libraries the same as secp, installing them to the lib/ directory and listing them in libbitcoinkernel.pc. That way the secp library would not be an outlier, the TARGET_OBJECTS calls would not be needed, and build would be lighter, with each .o object file added to one library file .a instead of two.
💬 ryanofsky commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1744149228)
Not important, but it would probably make sense for both `disable_network` and `temporary_rollback` variables to be `std::optional` instead of `std::unique_ptr` since they aren't passed around it would simplify the way they are initialized.
(https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1744149228)
Not important, but it would probably make sense for both `disable_network` and `temporary_rollback` variables to be `std::optional` instead of `std::unique_ptr` since they aren't passed around it would simplify the way they are initialized.
🚀 achow101 merged a pull request: "net: Favor peers from addrman over fetching seednodes"
(https://github.com/bitcoin/bitcoin/pull/29605)
(https://github.com/bitcoin/bitcoin/pull/29605)
💬 l0rinc commented on pull request "lint: Check for release note snippets in the wrong folder":
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1744155996)
> Seems overkill to import a dependency to check whether a dot is present in a string view or not
Maybe, in that case we can use glob patterns as well with `ls-files`:
```Rust
fn lint_doc_release_note_snippets() -> LintResult {
let result = check_output(git().args(["ls-files", "--", "doc/release-notes/release-notes-*.md", ":!doc/release-notes/*.*.md"]))?;
let snippet_notes = result.lines().collect::<Vec<_>>();
match snippet_notes.as_slice() {
[] => Ok(()),
...
(https://github.com/bitcoin/bitcoin/pull/30812#discussion_r1744155996)
> Seems overkill to import a dependency to check whether a dot is present in a string view or not
Maybe, in that case we can use glob patterns as well with `ls-files`:
```Rust
fn lint_doc_release_note_snippets() -> LintResult {
let result = check_output(git().args(["ls-files", "--", "doc/release-notes/release-notes-*.md", ":!doc/release-notes/*.*.md"]))?;
let snippet_notes = result.lines().collect::<Vec<_>>();
match snippet_notes.as_slice() {
[] => Ok(()),
...
🚀 achow101 merged a pull request: "contrib/signet/miner updates"
(https://github.com/bitcoin/bitcoin/pull/28417)
(https://github.com/bitcoin/bitcoin/pull/28417)
💬 achow101 commented on pull request "test: update satoshi_round function":
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2329615457)
ACK ec317bc44b0cb089419d809b5fef38ecb9423051
(https://github.com/bitcoin/bitcoin/pull/29566#issuecomment-2329615457)
ACK ec317bc44b0cb089419d809b5fef38ecb9423051
🚀 achow101 merged a pull request: "test: update satoshi_round function"
(https://github.com/bitcoin/bitcoin/pull/29566)
(https://github.com/bitcoin/bitcoin/pull/29566)
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744187608)
Not exactly, this looks more like a trampoline than simple hard-coded examples for just `ArithToUint256` and `UintToArith256`. Both sides depend on the same parameter, which seems circular to me-at least from a black-box perspective. However, I’m fine with this if you think it’s sufficient.
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1744187608)
Not exactly, this looks more like a trampoline than simple hard-coded examples for just `ArithToUint256` and `UintToArith256`. Both sides depend on the same parameter, which seems circular to me-at least from a black-box perspective. However, I’m fine with this if you think it’s sufficient.
💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744195573)
I don’t fully understand why we’re focusing on optimization at this stage. Why not keep both options for now, and if we later find that performance is an issue, remove the redundant ones based on their actual implementations? In my opinion, we’re still in the discovery phase, where having as much redundancy as we can afford is beneficial. Since fuzzing—and property-based testing in general—is black-box testing, we acknowledge that we don’t know the full input space. Our goal is to ensure that ce
...
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744195573)
I don’t fully understand why we’re focusing on optimization at this stage. Why not keep both options for now, and if we later find that performance is an issue, remove the redundant ones based on their actual implementations? In my opinion, we’re still in the discovery phase, where having as much redundancy as we can afford is beneficial. Since fuzzing—and property-based testing in general—is black-box testing, we acknowledge that we don’t know the full input space. Our goal is to ensure that ce
...
💬 remcoros commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2329687415)
> I think there is a small risk, that we will see a huge double-spending attempt in testnet4, because after block 00000000000000263393ce5f648afd53676f13d360cc9f264b89351623bf1242, there are transactions in the old chain, which are not picked by nodes, mining the new chain. Which means, that suddenly, when the deep reorg will revert hundreds of blocks, all of those transactions will suddenly become unconfirmed.
>
> So, if anyone has any coins, which were received after block number 42335, then
...
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2329687415)
> I think there is a small risk, that we will see a huge double-spending attempt in testnet4, because after block 00000000000000263393ce5f648afd53676f13d360cc9f264b89351623bf1242, there are transactions in the old chain, which are not picked by nodes, mining the new chain. Which means, that suddenly, when the deep reorg will revert hundreds of blocks, all of those transactions will suddenly become unconfirmed.
>
> So, if anyone has any coins, which were received after block number 42335, then
...
📝 fjahr opened a pull request: "test: Add coverage for dumptxoutset failure robustness"
(https://github.com/bitcoin/bitcoin/pull/30817)
This adds a test that checks that network activity is not suspended if dumptxoutset fails in the middle of its process which is implemented with the `NetworkDisable` RAII class. I would have liked to add coverage for the `TemporaryRollback` RAII class but that seems a lot more tricky since the failure needs to happen at some point after the rollback and on the scale of our test chain here I couldn't find a way to do it yet. This was requested by pablomartin4btc here: https://github.com/bitcoin/b
...
(https://github.com/bitcoin/bitcoin/pull/30817)
This adds a test that checks that network activity is not suspended if dumptxoutset fails in the middle of its process which is implemented with the `NetworkDisable` RAII class. I would have liked to add coverage for the `TemporaryRollback` RAII class but that seems a lot more tricky since the failure needs to happen at some point after the rollback and on the scale of our test chain here I couldn't find a way to do it yet. This was requested by pablomartin4btc here: https://github.com/bitcoin/b
...
💬 fjahr commented on pull request "rpc: dumptxoutset height parameter follow-ups (29553)":
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-2329699721)
Yo dawg, I heard you like follow-ups so I made a follow-up to my follow-up ;)
#30817 addresses the `std::optional` comment from @ryanofsky and adds a small test as suggested by @pablomartin4btc
(https://github.com/bitcoin/bitcoin/pull/30808#issuecomment-2329699721)
Yo dawg, I heard you like follow-ups so I made a follow-up to my follow-up ;)
#30817 addresses the `std::optional` comment from @ryanofsky and adds a small test as suggested by @pablomartin4btc
👍 ryanofsky approved a pull request: "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block"
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2280948816)
Code review ACK 6c419285720fe5a954e01f09cbe053a0f51bef47. Since last review just rebased, added suggested m_tip_block_cv notify call on shutdown, and added two new commits dropping g_best_block.
(https://github.com/bitcoin/bitcoin/pull/30409#pullrequestreview-2280948816)
Code review ACK 6c419285720fe5a954e01f09cbe053a0f51bef47. Since last review just rebased, added suggested m_tip_block_cv notify call on shutdown, and added two new commits dropping g_best_block.
💬 achow101 commented on pull request "cli: restrict multiple exclusive argument usage in bitcoin-cli":
(https://github.com/bitcoin/bitcoin/pull/30148#issuecomment-2329705872)
ACK c8e6771af002eaf15567794fcdc57fdb0e3fb140
(https://github.com/bitcoin/bitcoin/pull/30148#issuecomment-2329705872)
ACK c8e6771af002eaf15567794fcdc57fdb0e3fb140
👍 ryanofsky approved a pull request: "init: fix init fatal error on invalid negated option value"
(https://github.com/bitcoin/bitcoin/pull/30684#pullrequestreview-2280954609)
Code review ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef, just adding the suggested test since last review
(https://github.com/bitcoin/bitcoin/pull/30684#pullrequestreview-2280954609)
Code review ACK ee47ca29d6ef55650a0af63bca817c5d494f31ef, just adding the suggested test since last review
💬 achow101 commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#issuecomment-2329711694)
ACK 27c976d11a68d66db97d9a7a30c6a6a71c6ab586
(https://github.com/bitcoin/bitcoin/pull/30401#issuecomment-2329711694)
ACK 27c976d11a68d66db97d9a7a30c6a6a71c6ab586
💬 ryanofsky commented on pull request "init: fix init fatal error on invalid negated option value":
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2329714304)
May want to update the pr description to say this bug also when happens passing a double negated value, since the not_a_boolean case is really just a special case of that
(https://github.com/bitcoin/bitcoin/pull/30684#issuecomment-2329714304)
May want to update the pr description to say this bug also when happens passing a double negated value, since the not_a_boolean case is really just a special case of that