When using hosted runners in combination with cached docker images,
there is the possibility that the host runner image is updated,
rendering the linux-headers package (stored in the cached docker image)
incompatible.
Fix this by doing a re-install of the headers package in
03_test_script.sh.
If the underlying runner kernel has not changed thie has no effect, but
prevents the job from failing if it has.
To remove multiple occurances of the respository name, against which we
compare `${{ github.repository }}` to check if we should use Cirrus
Runners, introduce a helper job which can check a single environment
variable and output this as an input to subsequent jobs.
Forks can maintain a trivial patch of their repo name against the
`REPO_USE_CIRRUS_RUNNERS` variable in ci.yml if they have Cirrus Runners
of their own, which will then enable cache actions and docker build
cache to use Cirrus Cache.
It's not possible to use `${{ env.USE_CIRRUS_RUNNERS }}` in the
`runs-on:` directive as the context is not supported by GitHub.
If it was, this job would no longer be necessary.
Whilst the action cirruslabs/actions/cache will automatically set this
host, the docker `gha` build cache backend will not be aware of it.
Set the value here, which will later be used in the docker build args to
enable docker build cache on the cirrus cache.
This sets the build dir at build time so that Apple SDK gets installed
in the correct/expected location for the runtime to find it.
Co-authored-by: Max Edwards <youwontforgetthis@gmail.com>
Reverts: e87429a2d0
This was added in PR #31545 with the intention that self-hosted runners
might use it to save build cache.
As we are not using hosted runners with a registry build cache, the bulk
of this commit can be reverted, simply using the value of
$DOCKER_BUILD_CACHE_ARG in the script.
link: https://github.com/bitcoin/bitcoin/pull/31545
Using buildx is required to properly load the correct driver, for use
with registry caching. Neither build, nor BUILDKIT=1 currently do this
properly.
Use of `docker buildx build` is compatible with podman.
Another action to reduce boilerplate in the main ci.yml file.
This action will set up a docker builder compatible with caching build
layers to a container registry using the `gha` build driver.
It will then configure the docker build cache args.
If set, Cirrus runners will be used on pushes to, and pull requests
against, this repository.
Forks can set this if they have their own cirrus runners.
Add "Restore" and "Save" caching actions.
These actions reduce boilerplate in the main ci.yml configuration file.
These actions are implemented so that caches will be saved on `push`
only.
When a pull request is opened it will cache hit on the caches from the
lastest push, or in the case of depends will hit on any matching depends
hash, falling back to partial matches.
Depends caches are hashed using
`$(git ls-tree HEAD depends "ci/test/$FILE_ENV" | sha256sum | cut -d' ' -f1)`
and this hash is passed in as an input to the actions. This means we
direct cache hit in cases where depends would not be re-built, otherwise
falling back to a partial match.
Previous releases cache is hashed similarly to depends, but using the
test/get_previous_releases.py file.
The cirruslabs cache action will fallback transparently to GitHub's
cache in the case that the job is not being run on a Cirrus Runner,
making these compatible with running on forks (on free GH hardware).
b7b249d3ad Revert "[refactor] rewrite vTxHashes as a vector of CTransactionRef" (Anthony Towns)
b9300d8d0a Revert "refactor: Simplify `extra_txn` to be a vec of CTransactionRef instead of a vec of pair<Wtxid, CTransactionRef>" (Anthony Towns)
df5a50e5de bench/blockencodings: add compact block reconstruction benchmark (Anthony Towns)
Pull request description:
Reconstructing compact blocks is on the hot path for block relay, so revert changes from #28391 and #29752 that made it slower. Also add a benchmark to validate reconstruction performance, and a comment giving some background as to the approach.
ACKs for top commit:
achow101:
ACK b7b249d3ad
polespinasa:
lgtm code review and tested ACK b7b249d3ad
cedwies:
code-review ACK b7b249d
davidgumberg:
crACK b7b249d3ad
instagibbs:
ACK b7b249d3ad
Tree-SHA512: dc266e7ac08281a5899fb1d8d0ad43eb4085f8ec42606833832800a568f4a43e3931f942d4dc53cf680af620b7e893e80c9fe9220f83894b4609184b1b3b3b42
493ba0f688 threading: reduce the scope of lock in getblocktemplate (kevkevinpal)
Pull request description:
This change was motivated by https://github.com/bitcoin/bitcoin/pull/32592#discussion_r2294770722
It does exactly what is said in the comment. Reducing the scope of the lock by a bit before it is needed
ACKs for top commit:
stickies-v:
re-ACK 493ba0f688
maflcko:
lgtm ACK 493ba0f688
Tree-SHA512: aa3a21ef3da6be6c0af78aa2dda61ee21c3f6d4d9c897413dba9e7d7d2a91e9e069bbc6b6684b45aadaa28d8603dd310f2c2d2e58c31bb4d864204e468fefaf1
509ffea40a ci: return to using dash in CentOS job (fanquake)
Pull request description:
`dash` is available again: https://bugzilla.redhat.com/show_bug.cgi?id=2335416.
ACKs for top commit:
maflcko:
lgtm ACK 509ffea40a
davidgumberg:
ACK 509ffea40a
janb84:
crACK 509ffea40a
Tree-SHA512: c57194b6158f6453cadb2487be232af5e37aa2234852f04a76fc80909fbfa48c7f8dd30e7be41be67dedb7ec4886930e165fdbaf746d358bb94c6ccc49d6bde6
46ca7712cb threading: remove unused template instantiations (Cory Fields)
b537a6a6db threading: remove obsolete critsect macros (Cory Fields)
0d0e0a39b4 threading: use a reverse lock rather than manual critsect macros (Cory Fields)
3ddd554d31 tests: Add Assertions in reverse_lock tests to exercise thread-safety annotations (Cory Fields)
c88b1cbf57 tests: get rid of remaining manual critsect usage (Cory Fields)
Pull request description:
Now that #32467 is merged, the only remaining usage of our old `CRITICAL_SECTION` macros (other than tests) is in `getblocktemplate()` and it can safely be replaced with a `REVERSE_LOCK`.
This PR makes that replacement, replaces the old `CRITICAL_SECTION` macro usage in tests, then deletes the macros themselves.
~While testing this a few weeks ago, I noticed that `REVERSE_LOCK` does not currently work properly with our thread-safety annotations as after the `REVERSE_LOCK` is acquired, clang still believes that the mutex is locked. #32465 fixes this problem. Without that fix, this PR would potentially allow a false-negative if code were added in the future to this chunk of `getblocktemplate` which required `cs_main` to be locked.~
~I added a test for the reverse lock here in the form of a compiler warning in `reverselock_tests.cpp` to simulate that possibility. This PR will therefore cause a new warning (and should fail a warnings-as-errors ci check) until #32465 is merged and this is rebased on top of it.~
Edit: Rebased on top of #32465, so this should now pass tests.
ACKs for top commit:
maflcko:
review ACK 46ca7712cb📌
fjahr:
Code review ACK 46ca7712cb
TheCharlatan:
ACK 46ca7712cb
furszy:
ACK 46ca7712cb
Tree-SHA512: 5e423c8539ed5ddd784f5c3657bbd63be509d54942c25149f04e3764bcdf897bebf655553338d5af7b8c4f546fc1d4dd4176c2bce6f4683e76ae4bb91ba2ec80
a602f6fb7b test: index with an unclean restart after a reorg (Martin Zumsande)
01b95ac6f4 index: don't commit state in BaseIndex::Rewind (Martin Zumsande)
Pull request description:
The committed state of an index should never be ahead of the flushed chainstate.
Otherwise, in the case of an unclean shutdown, the blocks necessary to revert
from the prematurely committed state are not be available, which would corrupt the coinstatsindex in particular.
Instead, the index state will be committed with the next ChainStateFlushed notification.
Fixes#33208
ACKs for top commit:
achow101:
ACK a602f6fb7b
stickies-v:
re-ACK a602f6fb7b
Tree-SHA512: 2559ea3fe066caf746a54ad7daac5031332f3976848e937c3dc8b35fa2ce925674115d8742458bf3703b3916f04f851c26523b6b94aeb1da651ba5a1b167a419
1c3db0ed8e doc: use new block_to_connect parameter name (stickies-v)
Pull request description:
The parameter name was previously changed from `pblock` to `block_to_connect` in 9ba1fff29e, without updating the documentation.
Addresses https://github.com/bitcoin/bitcoin/pull/33078#discussion_r2279914775.
ACKs for top commit:
purpleKarrot:
ACK 1c3db0ed8e
janb84:
ACK 1c3db0ed8e
musaHaruna:
ACK [1c3db0e](1c3db0ed8e)
Tree-SHA512: 8b12243f1d9e5586e487dd705dc5b40ff12025bb5539eb4195f7fde4df38a9fe8eb0a9570a72f9463a2420f7307358409804fcb23bb73e32ff691ac4ef5bc35a
de65c86572 doc: capnproto instruction for Alpine and Arch (Sjors Provoost)
49d1a1a363 doc: add capnproto-devel to Fedora build instruction (Sjors Provoost)
eab5518913 doc: mark bitcoin-{node,gui} installed in files.md (Sjors Provoost)
2a815d126b doc: link to capnp version bump PR (Sjors Provoost)
Pull request description:
- have `dependencies.md` link to the PR that updated the capnp version: https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2290393840
ACKs for top commit:
maflcko:
lgtm ACK de65c86572
janb84:
re ACK de65c86572
Tree-SHA512: 842d7a89ef18a8c597ca05720c41a72e67500bc93430cf2c3b074cb2f4b936f1df58b5b1e99010e1ea5c1f8a9f8875fb9c20398f915feeacecee9b2fed3cb03c
966666de9a doc: Remove wrong and redundant doxygen tag (MarcoFalke)
Pull request description:
`param@[in]` is not a valid doxygen tag. Also, no other function in this file uses the annotations, and they are redundant with the line above, so just remove them in `feerate` to fix all issues.
In other places, fix them.
ACKs for top commit:
cedwies:
ACK 966666d
janb84:
ACK 966666de9a
pablomartin4btc:
ACK 966666de9a
w0xlt:
ACK 966666de9a
Tree-SHA512: fcb6aa75c0f03b36f3caad023854ba276e0335cf47908a77006e182633b6a68f7b7d3115ef9fb97d143ca23730def05550f970265bb1fde97594ba68e724bde9
7392b8b084 miner: clamp options instead of asserting (Pieter Wuille)
Pull request description:
The `BlockAssembler::ClampOptions` function currently doesn't actually clamp most of the provided settings, but asserts that some are in range. This made sense while it was a purely internal interface.
However, with the mining IPC interface exposed in #30510, these options are now externally accessible, and it is not entirely intuitive how to set them. In particular, calling `Mining::createNewBlock` with a default-constructed `BlockCreateOptions` will right now instantly crash the bitcoin node.
This isn't a security issue, as the IPC interface is considered trusted, but it is highly unexpected I think, and rather unergonomical to have the node crash while developing against the interface.
An alternative would be exposing a way for the interface to return a failure, but I think in this case, just correcting to reasonable values is acceptable.
ACKs for top commit:
Sjors:
ACK 7392b8b084
achow101:
ACK 7392b8b084
stickies-v:
ACK 7392b8b084
ryanofsky:
Code review ACK 7392b8b084. I think ideally this would throw an exception and return a clear error to the caller, or maybe log as stickies suggested, but clamping is much better than crashing.
Tree-SHA512: 7a1e05b68edbf57beb682ee63e27666f42af6a2b70a81874d368a2cb10d107a589e0a388658c1039330b8cc9f6048479870095a9d552ca387a250ac118c1abf2
The committed state of an index should never
be ahead of the flushed chainstate. Otherwise, in the case
of an unclean shutdown, the blocks necessary to revert
from the prematurely committed state would not be
available, which would corrupt the coinstatsindex in particular.
Instead, the index state will be committed with the next
ChainStateFlushed notification.
