📝 TheCharlatan opened a pull request: "kernel: Remove shutdown from kernel library"
(https://github.com/bitcoin/bitcoin/pull/27711)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". Together with #27576 this should complete step 2.
---
This removes the shutdown files from the kernel library.
Based on #27636. Actual commits start after the commit titled `commits after this are non-base`.
(https://github.com/bitcoin/bitcoin/pull/27711)
This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". Together with #27576 this should complete step 2.
---
This removes the shutdown files from the kernel library.
Based on #27636. Actual commits start after the commit titled `commits after this are non-base`.
📝 theStack opened a pull request: "test: p2p: check misbehavior for non-continuous headers messages"
(https://github.com/bitcoin/bitcoin/pull/27712)
This PR adds missing test coverage for a peer sending a `headers` message where the headers don't connect to each other, which should be treated as misbehaving (not disconnecting though, as the score increase is only 20). The relevant code path is `PeerManagerImpl::ProcessHeadersMessage` -> `PeerManagerImpl::CheckHeadersPoW` -> `PeerManagerImpl::CheckHeadersAreContinuous`:
https://github.com/bitcoin/bitcoin/blob/17acb2782a66f033238340e4fb81009e7f79e97a/src/net_processing.cpp#L2415-L2419
ht
...
(https://github.com/bitcoin/bitcoin/pull/27712)
This PR adds missing test coverage for a peer sending a `headers` message where the headers don't connect to each other, which should be treated as misbehaving (not disconnecting though, as the score increase is only 20). The relevant code path is `PeerManagerImpl::ProcessHeadersMessage` -> `PeerManagerImpl::CheckHeadersPoW` -> `PeerManagerImpl::CheckHeadersAreContinuous`:
https://github.com/bitcoin/bitcoin/blob/17acb2782a66f033238340e4fb81009e7f79e97a/src/net_processing.cpp#L2415-L2419
ht
...
💬 hebasto commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1556169237)
> > Looks like iwyu doesn't work with qt?
>
> We might have to provide a mapping file, for it to work sanely? i.e [include-what-you-use/include-what-you-use@`master`/qt5_11.imp](https://github.com/include-what-you-use/include-what-you-use/blob/master/qt5_11.imp?rgh-link-date=2023-05-17T13%3A29%3A36Z).
Added in https://github.com/bitcoin/bitcoin/pull/27710.
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1556169237)
> > Looks like iwyu doesn't work with qt?
>
> We might have to provide a mapping file, for it to work sanely? i.e [include-what-you-use/include-what-you-use@`master`/qt5_11.imp](https://github.com/include-what-you-use/include-what-you-use/blob/master/qt5_11.imp?rgh-link-date=2023-05-17T13%3A29%3A36Z).
Added in https://github.com/bitcoin/bitcoin/pull/27710.
💬 theStack commented on pull request "test: p2p: check misbehavior for non-continuous headers messages":
(https://github.com/bitcoin/bitcoin/pull/27712#issuecomment-1556180004)
(Force-pushed with a simpler variant that just deletes a random block header in the middle of a valid `headers` message instead of messing around with `hashPrevBlock`, which needed to re-grind the header for valid PoW. The previous version looked like this:
```
# modify previous block hash of an arbitrary sequent block header to break link
modified_block_header = block_headers[random.randrange(1, len(block_headers))]
modified_block_header.hashPrevBlock ^= 1
#
...
(https://github.com/bitcoin/bitcoin/pull/27712#issuecomment-1556180004)
(Force-pushed with a simpler variant that just deletes a random block header in the middle of a valid `headers` message instead of messing around with `hashPrevBlock`, which needed to re-grind the header for valid PoW. The previous version looked like this:
```
# modify previous block hash of an arbitrary sequent block header to break link
modified_block_header = block_headers[random.randrange(1, len(block_headers))]
modified_block_header.hashPrevBlock ^= 1
#
...
📝 hebasto converted_to_draft a pull request: "ci, iwyu: Update mappings"
(https://github.com/bitcoin/bitcoin/pull/27710)
The first commit has been pulled from https://github.com/bitcoin/bitcoin/pull/26766.
The second commit significantly reduces false warnings for unit tests. From the Boost.Test's [point of view](https://github.com/boostorg/test/blob/d2895ebfdfdf16074c58c9801d53e190c4654fcb/include/boost/test/unit_test.hpp#L11), the `boost/test/unit_test.hpp` header:
```
... should be the only header necessary to include to start using the framework
```
A note: IWYU's [`boost-1.75-all.imp`](https://github
...
(https://github.com/bitcoin/bitcoin/pull/27710)
The first commit has been pulled from https://github.com/bitcoin/bitcoin/pull/26766.
The second commit significantly reduces false warnings for unit tests. From the Boost.Test's [point of view](https://github.com/boostorg/test/blob/d2895ebfdfdf16074c58c9801d53e190c4654fcb/include/boost/test/unit_test.hpp#L11), the `boost/test/unit_test.hpp` header:
```
... should be the only header necessary to include to start using the framework
```
A note: IWYU's [`boost-1.75-all.imp`](https://github
...
📝 sbhuiyan20 opened a pull request: "Create Genarsative Ai"
(https://github.com/bitcoin/bitcoin/pull/27713)
<!--
*** 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 new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/27713)
<!--
*** 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 new tests that improv
...
✅ fanquake closed a pull request: "Create Genarsative Ai"
(https://github.com/bitcoin/bitcoin/pull/27713)
(https://github.com/bitcoin/bitcoin/pull/27713)
📝 fanquake locked a pull request: "Create Genarsative Ai"
(https://github.com/bitcoin/bitcoin/pull/27713)
<!--
*** 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 new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/27713)
<!--
*** 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 new tests that improv
...
💬 hebasto commented on pull request "ci: Update "Win64 native" task":
(https://github.com/bitcoin/bitcoin/pull/26891#issuecomment-1556187285)
FWIW, the recent [ccache 4.8.1](https://ccache.dev/releasenotes.html#_ccache_4_8_1) seems broken. Note the cacheable calls ratio:
```
Cacheable calls: 264 / 617 (42.79%)
Hits: 264 / 264 (100.0%)
Direct: 264 / 264 (100.0%)
Preprocessed: 0 / 264 ( 0.00%)
Misses: 0 / 264 ( 0.00%)
Uncacheable calls: 353 / 617 (57.21%)
Local storage:
Cache size (GB): 0.2 / 0.2 (99.87%)
Hits: 264 / 264 (100.0%)
Misses: 0 / 264 ( 0.00%
...
(https://github.com/bitcoin/bitcoin/pull/26891#issuecomment-1556187285)
FWIW, the recent [ccache 4.8.1](https://ccache.dev/releasenotes.html#_ccache_4_8_1) seems broken. Note the cacheable calls ratio:
```
Cacheable calls: 264 / 617 (42.79%)
Hits: 264 / 264 (100.0%)
Direct: 264 / 264 (100.0%)
Preprocessed: 0 / 264 ( 0.00%)
Misses: 0 / 264 ( 0.00%)
Uncacheable calls: 353 / 617 (57.21%)
Local storage:
Cache size (GB): 0.2 / 0.2 (99.87%)
Hits: 264 / 264 (100.0%)
Misses: 0 / 264 ( 0.00%
...
💬 ddykeman1 commented on issue "crash on macOS 12.6.5":
(https://github.com/bitcoin-core/gui/issues/731#issuecomment-1556206697)
Not sure how to do that at the moment
(https://github.com/bitcoin-core/gui/issues/731#issuecomment-1556206697)
Not sure how to do that at the moment
💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199788121)
yeah thx, this is a remanent from a previous version.
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199788121)
yeah thx, this is a remanent from a previous version.
💬 furszy commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199796712)
> Could the commit message clarify if any behavior is changing here? Would be good to label it a refactoring and say it does not change behavior if nothing is changing, and say what is changing otherwise.
yeah sure. https://github.com/bitcoin/bitcoin/commit/704c9e4cf67468708db655226a78489b92ef0523 introduces no behavior change.
> Also, it seems like new code that is added here just gets moved / deleted later in the PR in the last commit "index: verify blocks data existence only once" (http
...
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199796712)
> Could the commit message clarify if any behavior is changing here? Would be good to label it a refactoring and say it does not change behavior if nothing is changing, and say what is changing otherwise.
yeah sure. https://github.com/bitcoin/bitcoin/commit/704c9e4cf67468708db655226a78489b92ef0523 introduces no behavior change.
> Also, it seems like new code that is added here just gets moved / deleted later in the PR in the last commit "index: verify blocks data existence only once" (http
...
💬 TheCharlatan commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1556263037)
Approach NACK.
I get this - wasting time waiting for confirmations is more than annoying. Its effect usually lingers, because you loose context in the meantime and it slows down development in your entire organization. However, I also think supporting options that might lead to clients running disagreeing consensus rules on the same network is dangerous and might annoy developers in fresh ways. I would support the approach laid out by ajtowns in [this comment](https://github.com/bitcoin/bitc
...
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1556263037)
Approach NACK.
I get this - wasting time waiting for confirmations is more than annoying. Its effect usually lingers, because you loose context in the meantime and it slows down development in your entire organization. However, I also think supporting options that might lead to clients running disagreeing consensus rules on the same network is dangerous and might annoy developers in fresh ways. I would support the approach laid out by ajtowns in [this comment](https://github.com/bitcoin/bitc
...
👋 hebasto's pull request is ready for review: "ci, iwyu: Update mappings"
(https://github.com/bitcoin/bitcoin/pull/27710)
(https://github.com/bitcoin/bitcoin/pull/27710)
💬 benthecarman commented on pull request "Allow configuring target block time for a signet":
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1556271482)
> Approach NACK.
>
> I get this - wasting time waiting for confirmations is more than annoying. Its effect usually lingers, because you loose context in the meantime and it slows down development in your entire organization. However, I also think supporting options that might lead to clients running disagreeing consensus rules on the same network is dangerous and might annoy developers in fresh ways. I would support the approach laid out by ajtowns in [this comment](https://github.com/bitcoin
...
(https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1556271482)
> Approach NACK.
>
> I get this - wasting time waiting for confirmations is more than annoying. Its effect usually lingers, because you loose context in the meantime and it slows down development in your entire organization. However, I also think supporting options that might lead to clients running disagreeing consensus rules on the same network is dangerous and might annoy developers in fresh ways. I would support the approach laid out by ajtowns in [this comment](https://github.com/bitcoin
...
💬 mzumsande commented on pull request "index: improve initialization and pruning violation check":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199824807)
> Do you think the the main benefit of this commit is cleaner code, and the worse error detection a side effect? Or are there other benefits to this commit?
I think one minor downside could be that the indexes will be available a few minutes later because loadblk doesn't only do reindexing etc., it also loads the mempool - this could take several minutes depending on mempool size and there isn't really a reason why the indexes shouldn't be available before that has finished. This is already t
...
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1199824807)
> Do you think the the main benefit of this commit is cleaner code, and the worse error detection a side effect? Or are there other benefits to this commit?
I think one minor downside could be that the indexes will be available a few minutes later because loadblk doesn't only do reindexing etc., it also loads the mempool - this could take several minutes depending on mempool size and there isn't really a reason why the indexes shouldn't be available before that has finished. This is already t
...
🤔 mzumsande reviewed a pull request: "Parallel compact block downloads, take 3"
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1435468460)
Concept ACK, code looks good on first walkthrough.
I want to run this code with some additional logging for a day or two before acking.
In particular, I wonder if the fact that we'll now try additional peers will lead to redundancies / increased traffic under non-adversarial conditions where there's no staller but delivering the block just needs some time - especially when blocks contain a large number of txns that weren't in our mempool, or on slower networks like Tor. Did you consider th
...
(https://github.com/bitcoin/bitcoin/pull/27626#pullrequestreview-1435468460)
Concept ACK, code looks good on first walkthrough.
I want to run this code with some additional logging for a day or two before acking.
In particular, I wonder if the fact that we'll now try additional peers will lead to redundancies / increased traffic under non-adversarial conditions where there's no staller but delivering the block just needs some time - especially when blocks contain a large number of txns that weren't in our mempool, or on slower networks like Tor. Did you consider th
...
💬 mzumsande commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1199689963)
missing `assert` here
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1199689963)
missing `assert` here
💬 mzumsande commented on pull request "Parallel compact block downloads, take 3":
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1199822582)
nit: `already_in_flight` could be just a bool here
(https://github.com/bitcoin/bitcoin/pull/27626#discussion_r1199822582)
nit: `already_in_flight` could be just a bool here
🤔 mzumsande reviewed a pull request: "rpc: Add importmempool RPC"
(https://github.com/bitcoin/bitcoin/pull/27460#pullrequestreview-1435717252)
Should the rpc be executable before the mempool load from `ThreadImport` has finished? I tried importing another mempool before the mempool from the datadir was being loaded, and I didn't encounter any problems on first sight - but I'm not sure how useful that would be and I guess the final result would be dependent on races between the two mempools being loaded in parallel?
(https://github.com/bitcoin/bitcoin/pull/27460#pullrequestreview-1435717252)
Should the rpc be executable before the mempool load from `ThreadImport` has finished? I tried importing another mempool before the mempool from the datadir was being loaded, and I didn't encounter any problems on first sight - but I'm not sure how useful that would be and I guess the final result would be dependent on races between the two mempools being loaded in parallel?