📝 Dadudidas opened a pull request: "Rename SECURITY.md to DadudidaSECURITY.md"
(https://github.com/bitcoin/bitcoin/pull/27868)
Sicherheitslücke!
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or
...
(https://github.com/bitcoin/bitcoin/pull/27868)
Sicherheitslücke!
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or
...
💬 TheCharlatan commented on pull request "validation: Stricter assumeutxo error handling when renaming chainstates":
(https://github.com/bitcoin/bitcoin/pull/27862#discussion_r1227187324)
Just an observation on the behavior change here. In the current behavior, the function continues execution until it reaches `CompleteChainstateInitialization` further below. There it calls `options.check_interrupt()`, which will lead it to return `ChainstateLoadStatus::INTERRUPTED`. I think making it return a `ChainstateLoadStatus::FAILURE` now makes more sense, since it is not stopped due to an external interrupt.
(https://github.com/bitcoin/bitcoin/pull/27862#discussion_r1227187324)
Just an observation on the behavior change here. In the current behavior, the function continues execution until it reaches `CompleteChainstateInitialization` further below. There it calls `options.check_interrupt()`, which will lead it to return `ChainstateLoadStatus::INTERRUPTED`. I think making it return a `ChainstateLoadStatus::FAILURE` now makes more sense, since it is not stopped due to an external interrupt.
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1227189678)
Oh, I get what you mean. You’re right, the ancestor set feerates were incorrectly excluding some of the `high_fee` transactions. I amended the calculations and added the proposed additional checks. Thanks for catching that!
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1227189678)
Oh, I get what you mean. You’re right, the ancestor set feerates were incorrectly excluding some of the `high_fee` transactions. I amended the calculations and added the proposed additional checks. Thanks for catching that!
✅ hebasto closed a pull request: "Rename SECURITY.md to DadudidaSECURITY.md"
(https://github.com/bitcoin/bitcoin/pull/27868)
(https://github.com/bitcoin/bitcoin/pull/27868)
📝 hebasto locked a pull request: "."
(https://github.com/bitcoin/bitcoin/pull/27868)
Sicherheitslücke!
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or
...
(https://github.com/bitcoin/bitcoin/pull/27868)
Sicherheitslücke!
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or
...
💬 Xekyo commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1588026074)
I think that all follow-ups from #27021 have now been addressed in #26152.
(https://github.com/bitcoin/bitcoin/pull/27021#issuecomment-1588026074)
I think that all follow-ups from #27021 have now been addressed in #26152.
💬 TheCharlatan commented on pull request "validation: Stricter assumeutxo error handling when renaming chainstates":
(https://github.com/bitcoin/bitcoin/pull/27862#discussion_r1227196719)
This seems like a significant behavior change. The way I read this, `InvalidateCoinsDBOnDisk` is only called by `MaybeCompleteSnapshotValidation`. It's return type is ignored by its call site in `ConnectTip`, which I think would now lead us to skip over some code afterwards.
(https://github.com/bitcoin/bitcoin/pull/27862#discussion_r1227196719)
This seems like a significant behavior change. The way I read this, `InvalidateCoinsDBOnDisk` is only called by `MaybeCompleteSnapshotValidation`. It's return type is ignored by its call site in `ConnectTip`, which I think would now lead us to skip over some code afterwards.
👋 hebasto's pull request is ready for review: "Use `int32_t` type for most transaction size/weight values"
(https://github.com/bitcoin/bitcoin/pull/23962)
(https://github.com/bitcoin/bitcoin/pull/23962)
💬 Xekyo commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1588047051)
Rebased, added four commits for the follow-ups from #27021, cleaned up the commit messages, added @theStack’s wonderful topology overview for the transactions, built each commit separately to make sure all tests pass.
**Ready for review**
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1588047051)
Rebased, added four commits for the follow-ups from #27021, cleaned up the commit messages, added @theStack’s wonderful topology overview for the transactions, built each commit separately to make sure all tests pass.
**Ready for review**
👋 Xekyo's pull request is ready for review: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152)
(https://github.com/bitcoin/bitcoin/pull/26152)
💬 willcl-ark commented on pull request "test: handle failed `assert_equal()` assertions in bcc callback functions":
(https://github.com/bitcoin/bitcoin/pull/27831#issuecomment-1588058429)
ack 4798c4542b
I was a bit unfamilar with this python library so reviewed the bcc [docs](https://github.com/iovisor/bcc/blob/master/docs/tutorial_bcc_python_developer.md) and [reference guide](https://github.com/iovisor/bcc/blob/master/docs/reference_guide.md) before getting started...
Agree that these changes allow us to see errors propagated out into the test framework logs. I broke a few of the tests to ensure that this was the case.
nit: actually we currently dump errors to stderr,
...
(https://github.com/bitcoin/bitcoin/pull/27831#issuecomment-1588058429)
ack 4798c4542b
I was a bit unfamilar with this python library so reviewed the bcc [docs](https://github.com/iovisor/bcc/blob/master/docs/tutorial_bcc_python_developer.md) and [reference guide](https://github.com/iovisor/bcc/blob/master/docs/reference_guide.md) before getting started...
Agree that these changes allow us to see errors propagated out into the test framework logs. I broke a few of the tests to ensure that this was the case.
nit: actually we currently dump errors to stderr,
...
💬 TheCharlatan commented on pull request "blockstorage: Return on fatal flush errors":
(https://github.com/bitcoin/bitcoin/pull/27866#issuecomment-1588058565)
Re https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1475602374
> It also seems reasonable that some failed flushes like periodic flushes might not need to be fatal errors.
This is a good point, and I did not think of this before. A flush may fail once due to some sporadic error, but may succeed again at a later point in time. I'm still not sure though if it is really correct to ignore the flush error. If we fail to flush, but succeed in writing the block index, and then cras
...
(https://github.com/bitcoin/bitcoin/pull/27866#issuecomment-1588058565)
Re https://github.com/bitcoin/bitcoin/pull/27866#pullrequestreview-1475602374
> It also seems reasonable that some failed flushes like periodic flushes might not need to be fatal errors.
This is a good point, and I did not think of this before. A flush may fail once due to some sporadic error, but may succeed again at a later point in time. I'm still not sure though if it is really correct to ignore the flush error. If we fail to flush, but succeed in writing the block index, and then cras
...
💬 theuni commented on issue "Use semantic analysis in lint-logs.py":
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1588062158)
Still working on getting the plugin cleaned up, but in the meantime it pointed out what seems to have found a buggy case [here](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5433). Will PR a trivial fix for that.
(https://github.com/bitcoin/bitcoin/issues/27825#issuecomment-1588062158)
Still working on getting the plugin cleaned up, but in the meantime it pointed out what seems to have found a buggy case [here](https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L5433). Will PR a trivial fix for that.
💬 ccdle12 commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#discussion_r1227252081)
I reasoned since that this proposal wasn't introducing/editing arguably a core feature that it should be optional. Also setting it by default, I think clashes with how the functional tests currently run in Bitcoin since the sv2 server will respond to building a new template on best block change and will affect assertions on the state of the mempool in functional tests.
(https://github.com/bitcoin/bitcoin/pull/27854#discussion_r1227252081)
I reasoned since that this proposal wasn't introducing/editing arguably a core feature that it should be optional. Also setting it by default, I think clashes with how the functional tests currently run in Bitcoin since the sv2 server will respond to building a new template on best block change and will affect assertions on the state of the mempool in functional tests.
💬 ccdle12 commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#discussion_r1227252607)
This is true, I treated this PR as draft. Currently in development and testing against the SRI, all templates are assumed to be future so this would be updated if this PR gains interest for a complete implementation.
(https://github.com/bitcoin/bitcoin/pull/27854#discussion_r1227252607)
This is true, I treated this PR as draft. Currently in development and testing against the SRI, all templates are assumed to be future so this would be updated if this PR gains interest for a complete implementation.
💬 ccdle12 commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#discussion_r1227253132)
True, we can hard code it when these fields need to be serialized and add a comment above explaining why they are currently hard coded
(https://github.com/bitcoin/bitcoin/pull/27854#discussion_r1227253132)
True, we can hard code it when these fields need to be serialized and add a comment above explaining why they are currently hard coded
💬 achow101 commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1227262047)
In 99e28b1a58b9864e4f393a2a065ef63f12eb8b85 "Tighten requirements for adding elements to setBlockIndexCandidates"
Is this test case expected to fail with the changes in this commit or is it just expected to be incorrect? I tried re-enabling it and it passes.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1227262047)
In 99e28b1a58b9864e4f393a2a065ef63f12eb8b85 "Tighten requirements for adding elements to setBlockIndexCandidates"
Is this test case expected to fail with the changes in this commit or is it just expected to be incorrect? I tried re-enabling it and it passes.
💬 achow101 commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1227262861)
In 99e28b1a58b9864e4f393a2a065ef63f12eb8b85 "Tighten requirements for adding elements to setBlockIndexCandidates"
Is this check expected to fail? I tried uncommenting it and the test passes.
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1227262861)
In 99e28b1a58b9864e4f393a2a065ef63f12eb8b85 "Tighten requirements for adding elements to setBlockIndexCandidates"
Is this check expected to fail? I tried uncommenting it and the test passes.
💬 achow101 commented on pull request "Draft: rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1227265509)
In 99e28b1a58b9864e4f393a2a065ef63f12eb8b85 "Tighten requirements for adding elements to setBlockIndexCandidates"
This diff makes the test work, assuming that `TryAddBlockIndexCandidate` is working as it is supposed to be.
```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 8fab53c5c5..b9fe054a64 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@
...
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1227265509)
In 99e28b1a58b9864e4f393a2a065ef63f12eb8b85 "Tighten requirements for adding elements to setBlockIndexCandidates"
This diff makes the test work, assuming that `TryAddBlockIndexCandidate` is working as it is supposed to be.
```diff
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp
index 8fab53c5c5..b9fe054a64 100644
--- a/src/test/validation_chainstatemanager_tests.cpp
+++ b/src/test/validation_chainstatemanager_tests.cpp
@@
...
💬 LarryRuane commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#issuecomment-1588151534)
Force-pushed rebase for merge conflict
(https://github.com/bitcoin/bitcoin/pull/26697#issuecomment-1588151534)
Force-pushed rebase for merge conflict