Sync with the main repo #359
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…eMany() Instead of recursively calling `SerializeMany` and peeling off one argument at a time, use a fold expression. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations.
…izeMany() Instead of recursively calling `UnserializeMany` and peeling off one argument at a time, use a fold expression. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations.
This gets rid of unnecessarily creating a temporary object T() to call the right function.
This gets rid of unnecessarily creating a temporary object T() to call the right function.
This gets rid of unnecessarily creating a temporary object T() to call the right function.
This gets rid of unnecessarily creating a temporary object T() to call the right function.
703b758 qa: Close SQLite connection properly (Hennadii Stepanov) Pull request description: This PR is a follow-up for bitcoin/bitcoin#26462 that introduced a bug on Windows: ``` >test\functional\wallet_descriptor.py ... PermissionError: [WinError 32] The process cannot access the file because it is being used by another process: ... ``` From `sqlite3` Python module [docs](https://docs.python.org/3/library/sqlite3.html#how-to-use-the-connection-context-manager): > `Connection` object used as context manager only commits or rollbacks transactions, so the connection object should be closed manually. ACKs for top commit: MarcoFalke: lgtm ACK 703b758 theStack: utACK 703b758 Tree-SHA512: 35b1403507be06d1fc04e7e07ff56af5bcfe5013024671f0c1d9f3c41aacc4c777bcc6376ce82d720394e27450415d50ff5d5834ed388ec3f21503f86f1a42a5
…iables signed 92de74e refactor: Make more transaction size variables signed (Hennadii Stepanov) Pull request description: This PR is a continuation of bitcoin/bitcoin#23962 and it: - gets rid of two static casts, - addresses bitcoin/bitcoin#23962 (comment), - is useful for bitcoin/bitcoin#25972, see the failed ARM and multiprocess CI jobs. ACKs for top commit: MarcoFalke: lgtm ACK 92de74e 🥔 glozow: ACK 92de74e Tree-SHA512: 84225961af8e08439664e75661b98fe86560217e891e5633a28316bf248d88df317a0c6b5a5f6b03feb2b0e0fd40a1f91dd4a85a0610d567470805bf47a84487
Rather than using a bloom filter to track announced invs, simply allow a peer to request any tx that entered the mempool prior to the last INV message we sent them. This also obsoletes the UNCONDITIONAL_RELAY_DELAY.
We no longer have m_recently_announced_invs, so there is no need to add txids to m_tx_inventory_known_filter to dedupe that filter.
Check that peers can immediately request txs from blocks that have been reorged out and are now in our mempool.
Using `py.exe` launcher might by fragile depending on how Python was installed. Specifying the Python version explicitly fixes test errors like this: ``` RunCommandParseJSON error: process(py C:\Users\hebasto\bitcoin\test\functional\mocks\signer.py enumerate) returned 9009: Python was not found... ``` -BEGIN VERIFY SCRIPT- sed -i 's|"py "|"py -3 "|g' $(git grep -l '"py "' -- test/functional) -END VERIFY SCRIPT-
…ble` when possible 025fda0 fuzz: addrman, avoid `ConsumeDeserializable` when possible (brunoerg) Pull request description: Using specific functions like `ConsumeService`, `ConsumeAddress` and `ConsumeNetAddr` may be more effective than using `ConsumeDeserializable`. They always return some value while `ConsumeDeserializable` may return `std::nullopt`. E.g.: In this part of the code, if `op_net_addr` is `std::nullopt`, we basically generated the addresses (if so) unnecessarily, because we won't be able to use them: ```cpp std::vector<CAddress> addresses; LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) { const std::optional<CAddress> opt_address = ConsumeDeserializable<CAddress>(fuzzed_data_provider); if (!opt_address) { break; } addresses.push_back(*opt_address); } const std::optional<CNetAddr> opt_net_addr = ConsumeDeserializable<CNetAddr>(fuzzed_data_provider); if (opt_net_addr) { addr_man.Add(addresses, *opt_net_addr, std::chrono::seconds{ConsumeTime(fuzzed_data_provider, 0, 100000000)}); } ``` Also, if we are not calling `Add` effectively, it would also be affect other functions that may "depend" on it. ACKs for top commit: dergoegge: Code review ACK 025fda0 Tree-SHA512: 02450bec0b084c15ba0cd1cbdfbac067c8fea4ccf27be0c86d54e020f029a6c749a16d8e0558f9d6d35a7ca9db8916f180c872f09474702b5591129e9be0d192
…carriersize=2 tests faafc35 doc: Clarify that -datacarriersize applies to the full raw scriptPubKey, not the data push (MarcoFalke) 55550e7 test: Add -datacarriersize=2 tests (MarcoFalke) Pull request description: Clarify with a test that `-datacarriersize` applies to the serialized size of the scriptPubKey, not the size of the pushed data. So for example, * `-datacarriersize=2` will reject a `raw(6a01aa)`, even though only one byte is pushed * `-datacarriersize=0` (or `-datacarrier=0`) will reject a `raw(6a)`, even though no byte is pushed * `-datacarriersize=0` (or `-datacarrier=0`) will reject a `raw(6a00)`, even though zero bytes are pushed ACKs for top commit: ajtowns: ACK faafc35 instagibbs: ACK bitcoin/bitcoin@faafc35 Tree-SHA512: f01ace02798f596ac2a02461e9f2a6ef91b3b37c976ea0b3bc860e2d3efb0ace0fd8b779dd18249cee7f84ebbe5fd21d8506afd3a15edadc00b843ff3b4aacc7
Enable `bitcoin-unterminated-logprintf`. Co-authored-by: Cory Fields <cory-nospam-@coryfields.com>
fa47439 ci: Add missing linux-headers package to ASan task (MarcoFalke) fabaa85 ci: Move ASan USDT to persistent_worker (MarcoFalke) Pull request description: To run the USDT functional tests, the ASan task currently requires the container host to run the Ubuntu Lunar Linux kernel (or later). Cirrus CI is the only provider that allows to spin up full VMs with Ubuntu Lunar, however they will start to charge for all tasks (See slightly related discussion in bitcoin/bitcoin#28098). Since it is cheaper and recommended by Cirrus CI to just run a persistent worker, do that. Also, using a persistent worker allows to make use of the docker image cache. ACKs for top commit: hebasto: ACK fa47439, I have reviewed the code and it looks OK. Tree-SHA512: afd084ab1b56cbc3fa44d4611aaa01ec21c1d80aedf1f5f1bc4b8b3d1bd08095e0c7fcea7a3e6ec4b6cd97d01e97ee86061eb84a5e2c7e7195ce02a186254900
Connman uses this new map to keep a count of active OUTBOUND_FULL_RELAY and MANUAL connections. Unused until next commit. Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
If a peer is the only one of its network, protect it from eviction. This improves the diversity of outbound connections with respect to reachable networks. Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
Diversify outbound connections with respect to networks: Every ~5 minutes, try to add an extra connection to a reachable network which we currently don't have a connection to. This is done defensively - only try management with respect to networks after all existing outbound slots are filled. The resulting situation with an extra outbound peer will be handled by the extra outbound eviction logic, which protects peers from eviction if they are the only ones for their network. Co-authored-by: Amiti Uttarwar <amiti@uttarwar.org>
It is incorrect to assert that `cache.HaveCoin()` will always be `true` if `backend.HaveCoin()` is. The coin could well have been marked as spent in the cache but not yet flushed, in which case `cache.HaveCoin()` would return `false`. Note this was never hit because `exists_using_have_coin_in_backend` is currently never `true` (it's the default implementation of `CCoinsView`. However this might change if we were to add a target where the backend is a `CCoinsViewDB`.
f054bd0 refactor: use "if constexpr" in std::vector's Unserialize() (Martin Leitner-Ankerl) 088caa6 refactor: use "if constexpr" in std::vector's Serialize() (Martin Leitner-Ankerl) 0fafaca refactor: use "if constexpr" in prevector's Unserialize() (Martin Leitner-Ankerl) c8839ec refactor: use "if constexpr" in prevector's Serialize() (Martin Leitner-Ankerl) 1403d18 refactor: use fold expressions instead of recursive calls in UnserializeMany() (Martin Leitner-Ankerl) bd08a00 refactor: use fold expressions instead of recursive calls in SerializeMany() (Martin Leitner-Ankerl) Pull request description: This simplifies the serialization code a bit and should also make it a bit faster. * use fold expressions instead of recursive calls. This simplifies the code, makes it most likely faster because it reduces the number of function calls, and compiles faster because there are fewer template instantiations. * use `if constexpr` instead of unnecessarily creating a temporary object only to call the right overload. This is used for `std::vector` and `prevector` serialization. ACKs for top commit: MarcoFalke: only change is to add a missing `&`. lgtm, re-ACK f054bd0 📦 jonatack: ACK f054bd0 sipa: utACK f054bd0 john-moffett: ACK f054bd0 Tree-SHA512: 0417bf2d6be486c581732297945449211fc3481bac82964e27628b38ef55a47dfa58d730148aeaf1b19fa8eb1076489cc646ceebb178162a9afa59034601501d
…hains 13eb8aa doc: Release notes for testnet defaulting to -acceptnonstdtxn=0 (Anthony Towns) e1dc15d config: default acceptnonstdtxn=0 on all chains (Anthony Towns) Pull request description: Changes `-acceptnonstxtxn` to default to 0 on testnet, matching the other chains. Allowing non-standard txs on testnet by default contributed to the difficulties RSK described in #26348: "We see that there are two script paths and, to reduce the script size, a single CHECKMULTISIG is used for the two paths, separating the signer count from the CHECKMULTISIG opcode. This script worked on testnet, because it lacks the standard checks performed in Mainnet." ACKs for top commit: MarcoFalke: lgtm ACK 13eb8aa sipa: utACK 13eb8aa instagibbs: utACK bitcoin/bitcoin@13eb8aa theStack: Code-review ACK 13eb8aa Tree-SHA512: eff7a3f9fc9b94003a730beb96e6f3399bc8b8e93fde4b15f20a11eda61d9a3e076f4423989f98b794b32681abecbc3756a54cd0d37b136e2fb2ffbb47ee7774
…ith importlib.metadata 6c008a2 script: replace deprecated pkg_resources with importlib.metadata (Jon Atack) Pull request description: Running our python linter with a recent python and the latest release of setuptools [v68.1.2](https://setuptools.pypa.io/en/stable/history.html): ``` $ python3 --version Python 3.11.5 $ ./test/lint/lint-python.py:12: DeprecationWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html import pkg_resources ``` Using `pkg_resources` was [deprecated](pypa/setuptools#3843) earlier in [v67.5.0](https://setuptools.pypa.io/en/stable/history.html#id55): "Although pkg_resources has been discouraged for use, some projects still consider pkg_resources viable for usage. This change makes it clear that pkg_resources should not be used, emitting a DeprecationWarning when imported." The `importlib.metadata` library requires Python 3.8, which is currently our minimum-supported Python version. For more details about `importlib.metadata` and the two methods imported and used here, see: - https://docs.python.org/3/library/importlib.metadata.html - https://importlib-metadata.readthedocs.io/en/latest/api.html#importlib_metadata.metadata - https://importlib-metadata.readthedocs.io/en/latest/api.html#importlib_metadata.PackageNotFoundError ACKs for top commit: MarcoFalke: lgtm ACK 6c008a2 (review only, did not test) Tree-SHA512: f5258d37043fcc9744f85641a60a3395ad43822c72d030dea8c39fa7f48ec3d7790cdeeb832f96e8f38046adb7c62fbc577c975ef0c77c8047c0c8f2353ce540
This reverts commit 28ab1ee.
This reverts commit 9fc7bce.
This reverts commit fde1c2f.
This reverts commit 380e74c.
This reverts commit f426430.
This reverts commit 3df22b9.
This reverts commit e4cc7ec.
This reverts commit dc600c2.
This reverts commit 743e9cf.
This reverts commit e66f0bd.
This reverts commit c8f4e03.
This reverts commit 475c63e.
|
After the last push, the CI The UPD. Or is it just an intermittent GitHub issue? |
UPD. It still happens. |
The following changes considered: - bitcoin/bitcoin#27419 - bitcoin/bitcoin#27491 - bitcoin/bitcoin#27576 - bitcoin/bitcoin#27636
It should be re-enabled once the issue with the depends cache is resolved. See: bitcoin/bitcoin#28365.
|
Going to merge this PR to unlock migration from non-free Cirrus CI to free GitHub Actions. Hoping someone will make post-merge review soon. |
|
utACK 14ddf61. ACK merge commit b7cdd31 Light code review:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sync with the main repo up to the latest bitcoin/bitcoin@ab42b2e, which includes the recent changes in the CI.
There is no downloadable artifacts support for now. It will be done in a separated PR(s).
Additionally:
modernize-use-default-member-initclang-tidy warnings.Guix builds: