🚀 fanquake merged a pull request: "Release: 30.0rc3 translations update"
(https://github.com/bitcoin/bitcoin/pull/33541)
(https://github.com/bitcoin/bitcoin/pull/33541)
💬 fanquake commented on pull request "[30.x] Backports & rc3":
(https://github.com/bitcoin/bitcoin/pull/33473#issuecomment-3370701596)
> Can you add #33229?
I'm not going to add that here, `rc3` is already late. There might be more multiprocess backporting done, so it could be included there, but that's also blocked on at least https://github.com/bitcoin-core/libmultiprocess/pull/222.
(https://github.com/bitcoin/bitcoin/pull/33473#issuecomment-3370701596)
> Can you add #33229?
I'm not going to add that here, `rc3` is already late. There might be more multiprocess backporting done, so it could be included there, but that's also blocked on at least https://github.com/bitcoin-core/libmultiprocess/pull/222.
⚠️ hodlinator opened an issue: "HeadersSync tracking issue"
(https://github.com/bitcoin/bitcoin/issues/33547)
End Goal: Make the headers sync phase quicker and less prone to fail, while improving implementation and test code.
The Bitcoin Optech Podcast episode for Newsletter 322 (https://bitcoinops.org/en/podcast/2024/10/01/) among other things prompted the (re)discovery of the possible improvement of caching headers so they only needed to be downloaded once. To avoid adding unnecessary resource demands on nodes, the number of `HeaderSyncState` instances created over time should ideally be made more pr
...
(https://github.com/bitcoin/bitcoin/issues/33547)
End Goal: Make the headers sync phase quicker and less prone to fail, while improving implementation and test code.
The Bitcoin Optech Podcast episode for Newsletter 322 (https://bitcoinops.org/en/podcast/2024/10/01/) among other things prompted the (re)discovery of the possible improvement of caching headers so they only needed to be downloaded once. To avoid adding unnecessary resource demands on nodes, the number of `HeaderSyncState` instances created over time should ideally be made more pr
...
🚀 fanquake merged a pull request: "[30.x] Backports & rc3"
(https://github.com/bitcoin/bitcoin/pull/33473)
(https://github.com/bitcoin/bitcoin/pull/33473)
🤔 janb84 reviewed a pull request: "doc: add coverage instrumentation hint to libFuzzer quickstart"
(https://github.com/bitcoin/bitcoin/pull/33536#pullrequestreview-3303634902)
NACK [Developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-fuzz-coverage) already has an extensive section on generating coverage. Maintaining the same documentation in 2 places is not ideal.
(https://github.com/bitcoin/bitcoin/pull/33536#pullrequestreview-3303634902)
NACK [Developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-fuzz-coverage) already has an extensive section on generating coverage. Maintaining the same documentation in 2 places is not ideal.
💬 maflcko commented on pull request "doc: add coverage instrumentation hint to libFuzzer quickstart":
(https://github.com/bitcoin/bitcoin/pull/33536#discussion_r2405551412)
not sure about recommending libfuzzer here. It has issues properly counting runs, so coverage will be off. See also https://github.com/bitcoin/bitcoin/blob/a33bd767a37dccf39a094d03c2f62ea81633410f/contrib/devtools/deterministic-fuzz-coverage/src/main.rs#L111-L115
(https://github.com/bitcoin/bitcoin/pull/33536#discussion_r2405551412)
not sure about recommending libfuzzer here. It has issues properly counting runs, so coverage will be off. See also https://github.com/bitcoin/bitcoin/blob/a33bd767a37dccf39a094d03c2f62ea81633410f/contrib/devtools/deterministic-fuzz-coverage/src/main.rs#L111-L115
💬 hebasto commented on pull request "Release: 30.0 translations update":
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3370903483)
@jesterhodl
> I resolved the 22 issues listed in the Polish translation
Thank you for your contribution! The updated Polish translation has been fetched from Transifex in https://github.com/bitcoin/bitcoin/pull/33541.
(https://github.com/bitcoin/bitcoin/pull/33275#issuecomment-3370903483)
@jesterhodl
> I resolved the 22 issues listed in the Polish translation
Thank you for your contribution! The updated Polish translation has been fetched from Transifex in https://github.com/bitcoin/bitcoin/pull/33541.
💬 mikekelly commented on issue "v30rc2 createrawtransaction unable to create txns with multiple OP_RETURNs":
(https://github.com/bitcoin/bitcoin/issues/33544#issuecomment-3370911419)
> Here's the offending lines
>
> https://github.com/bitcoin/bitcoin/blob/30.x/src/rpc/rawtransaction_util.cpp#L109-L111
@maflcko this is all that needs removing and replacing with a warning. I assume there are standardness tests that will start failing and need changing too. Not sure if I'll have time to get to this myself. Also concerned there may be dragons here and it's not as simple as it appears - what do you think @petertodd or @murchandamus ?
(https://github.com/bitcoin/bitcoin/issues/33544#issuecomment-3370911419)
> Here's the offending lines
>
> https://github.com/bitcoin/bitcoin/blob/30.x/src/rpc/rawtransaction_util.cpp#L109-L111
@maflcko this is all that needs removing and replacing with a warning. I assume there are standardness tests that will start failing and need changing too. Not sure if I'll have time to get to this myself. Also concerned there may be dragons here and it's not as simple as it appears - what do you think @petertodd or @murchandamus ?
💬 maflcko commented on issue "v30rc2 createrawtransaction unable to create txns with multiple OP_RETURNs":
(https://github.com/bitcoin/bitcoin/issues/33544#issuecomment-3370947247)
There is https://github.com/bitcoin/bitcoin/pull/32790/files, but it is still in draft and has not received any code-review, only a conceptual feedback.
(https://github.com/bitcoin/bitcoin/issues/33544#issuecomment-3370947247)
There is https://github.com/bitcoin/bitcoin/pull/32790/files, but it is still in draft and has not received any code-review, only a conceptual feedback.
💬 frankomosh commented on pull request "doc: add coverage instrumentation hint to libFuzzer quickstart":
(https://github.com/bitcoin/bitcoin/pull/33536#discussion_r2405646510)
thanks. wasn't aware of the run counting issues with LibFuzzer.
So I guess the best approach is to build without the fuzzer sanitizer ?
(https://github.com/bitcoin/bitcoin/pull/33536#discussion_r2405646510)
thanks. wasn't aware of the run counting issues with LibFuzzer.
So I guess the best approach is to build without the fuzzer sanitizer ?
💬 frankomosh commented on pull request "doc: add coverage instrumentation hint to libFuzzer quickstart":
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3370955199)
> NACK [Developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-fuzz-coverage) already has an extensive section on generating coverage. Maintaining the same documentation in 2 places is not ideal.
ok .
Would you atleast like to see a reference to the developer notes in this section(assume for someone who lands here first)?
(https://github.com/bitcoin/bitcoin/pull/33536#issuecomment-3370955199)
> NACK [Developer notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#compiling-for-fuzz-coverage) already has an extensive section on generating coverage. Maintaining the same documentation in 2 places is not ideal.
ok .
Would you atleast like to see a reference to the developer notes in this section(assume for someone who lands here first)?
💬 mikekelly commented on pull request "rpc, test: allow multiple data outputs in `createrawtransaction` & `createpsbt`":
(https://github.com/bitcoin/bitcoin/pull/32790#issuecomment-3370956828)
@yuvicc FYI - there are failing checks on this. If you make this build green then it could be proposed as part of v30 release.
(https://github.com/bitcoin/bitcoin/pull/32790#issuecomment-3370956828)
@yuvicc FYI - there are failing checks on this. If you make this build green then it could be proposed as part of v30 release.
💬 fanquake commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3370962466)
An `rc3` has been tagged: https://github.com/bitcoin/bitcoin/releases/tag/v30.0rc3.
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3370962466)
An `rc3` has been tagged: https://github.com/bitcoin/bitcoin/releases/tag/v30.0rc3.
💬 mikekelly commented on pull request "rpc, test: allow multiple data outputs in `createrawtransaction` & `createpsbt`":
(https://github.com/bitcoin/bitcoin/pull/32790#issuecomment-3370963517)
fwiw - I think if this is going in to 30.0 it would need some kind of warning to tell users that they may face propagation issues. If it's delayed to 30.1 then a warning probably isn't necessary.
(https://github.com/bitcoin/bitcoin/pull/32790#issuecomment-3370963517)
fwiw - I think if this is going in to 30.0 it would need some kind of warning to tell users that they may face propagation issues. If it's delayed to 30.1 then a warning probably isn't necessary.
💬 hebasto commented on issue "Implicit conversion from `fs::path` to `std::string` when constructing file streams":
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3370969802)
> I guess this means any fs::path would have to be explicitly cast to a std::fs::path, which seems fine to do.
This is one way of doing it.
> I guess you are asking how to deal with the fs linter?
This, and something like `operator std::string() const = delete;` should be added to `fs::path`.
(https://github.com/bitcoin/bitcoin/issues/33545#issuecomment-3370969802)
> I guess this means any fs::path would have to be explicitly cast to a std::fs::path, which seems fine to do.
This is one way of doing it.
> I guess you are asking how to deal with the fs linter?
This, and something like `operator std::string() const = delete;` should be added to `fs::path`.
💬 mikekelly commented on issue "v30rc2 createrawtransaction unable to create txns with multiple OP_RETURNs":
(https://github.com/bitcoin/bitcoin/issues/33544#issuecomment-3370970077)
@maflcko I suppose if it's not going into 30.0 then it wont need warnings. It's a shame to not have it in 30.0 but if the ship has sailed it is what it is.
(https://github.com/bitcoin/bitcoin/issues/33544#issuecomment-3370970077)
@maflcko I suppose if it's not going into 30.0 then it wont need warnings. It's a shame to not have it in 30.0 but if the ship has sailed it is what it is.
🤔 stickies-v reviewed a pull request: "validation: remove BLOCK_FAILED_CHILD"
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3303574618)
Approach ACK 2e040d8b3c861c96c536754a82f4352b635c4d01
Intuitively it seems helpful to track something like `BLOCK_FAILED_CHILD`, but since it's been unused for so long it seems the use case isn't really there. Block validation logic is complex and critical and simplifying it is worth the effort, so I think this is the way to go.
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3303574618)
Approach ACK 2e040d8b3c861c96c536754a82f4352b635c4d01
Intuitively it seems helpful to track something like `BLOCK_FAILED_CHILD`, but since it's been unused for so long it seems the use case isn't really there. Block validation logic is complex and critical and simplifying it is worth the effort, so I think this is the way to go.
💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2405483992)
In 8e6c195396304e3c66471e208b1cd59eea02c11c:
This logic seems to deal with some kind of block index corruption (e.g. unclean shutdown) where not all children of an invalid block have been marked as invalid on-disk.
Let's assume a chain of blocks `A -> B -> C`. Do we have strong guarantees that it's impossible that:
- `A` is marked `BLOCK_FAILED_VALID`
- `B` is marked `BLOCK_FAILED_CHILD`
- `C` is not marked invalid
In the above scenario, the `blockstorage.cpp` logic on `master` would
...
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2405483992)
In 8e6c195396304e3c66471e208b1cd59eea02c11c:
This logic seems to deal with some kind of block index corruption (e.g. unclean shutdown) where not all children of an invalid block have been marked as invalid on-disk.
Let's assume a chain of blocks `A -> B -> C`. Do we have strong guarantees that it's impossible that:
- `A` is marked `BLOCK_FAILED_VALID`
- `B` is marked `BLOCK_FAILED_CHILD`
- `C` is not marked invalid
In the above scenario, the `blockstorage.cpp` logic on `master` would
...
💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2405494993)
In 507b937eb39866c182b8a8939d2a71a80618f398: I think the `invalid_walk_tip` can (and if so, should) be kept local to the `while (true)` loop. Once that loop is finished, `invalid_walk_tip` should be equivalent to `pindex`? This better isolates the tip-rewinding logic.
Suggested diff (that also uses `{disconnected,new}_tip` and moved code closer to where it's used for readability):
<details>
<summary>git diff on 507b937eb3</summary>
```diff
diff --git a/src/validation.cpp b/src/vali
...
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2405494993)
In 507b937eb39866c182b8a8939d2a71a80618f398: I think the `invalid_walk_tip` can (and if so, should) be kept local to the `while (true)` loop. Once that loop is finished, `invalid_walk_tip` should be equivalent to `pindex`? This better isolates the tip-rewinding logic.
Suggested diff (that also uses `{disconnected,new}_tip` and moved code closer to where it's used for readability):
<details>
<summary>git diff on 507b937eb3</summary>
```diff
diff --git a/src/validation.cpp b/src/vali
...
💬 stickies-v commented on pull request "validation: remove BLOCK_FAILED_CHILD":
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2405502780)
In 2e040d8b3c861c96c536754a82f4352b635c4d01:
It seems like we're trying to cram two different operations into one:
1. ensuring that all descendants of an invalid block are marked as invalid, in case there was any corruption/interruption
2. phasing out now-deprecated `BLOCK_FAILED_CHILD` flags
The current approach doesn't look robust to me, i.e. it won't catch all `BLOCK_FAILED_CHILD` flags. Suggested alternative that is more robust and (imo) more readable:
<details>
<summary>git diff
...
(https://github.com/bitcoin/bitcoin/pull/32950#discussion_r2405502780)
In 2e040d8b3c861c96c536754a82f4352b635c4d01:
It seems like we're trying to cram two different operations into one:
1. ensuring that all descendants of an invalid block are marked as invalid, in case there was any corruption/interruption
2. phasing out now-deprecated `BLOCK_FAILED_CHILD` flags
The current approach doesn't look robust to me, i.e. it won't catch all `BLOCK_FAILED_CHILD` flags. Suggested alternative that is more robust and (imo) more readable:
<details>
<summary>git diff
...