f19fdd47a6 test: add test for CChainState::ResizeCoinsCaches() (James O'Beirne)
8ac3ef4699 add ChainstateManager::MaybeRebalanceCaches() (James O'Beirne)
f36aaa6392 Add CChainState::ResizeCoinsCaches (James O'Beirne)
b223111da2 txdb: add CCoinsViewDB::ChangeCacheSize (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
In the assumeutxo implementation draft (#15056), once a UTXO snapshot is loaded, a new chainstate object is created after initialization. This means that we have to reclaim some of the cache that we've allocated to the original chainstate (per `dbcache=`) to repurpose for the snapshot chainstate.
Furthermore, it makes sense to have different cache allocations depending on which chainstate is more active. While the snapshot chainstate is working to get to the network tip (and the background validation chainstate is idle), it makes sense that the snapshot chainstate should have the majority of cache allocation. And contrariwise once the snapshot has reached network tip, most of the cache should be given to the background validation chainstate.
This set of changes (detailed in the commit messages) allows us to dynamically resize the various coins caches. None of the functionality introduced here is used at the moment, but will be in the next AU PR (which introduces `ActivateSnapshot`).
`ChainstateManager::MaybeRebalanceCaches()` defines the (somewhat normative) cache allocations between the snapshot and background validation chainstates. I'd be interested in feedback if anyone has thoughts on the proportions I've set there.
ACKs for top commit:
ajtowns:
weak utACK f19fdd47a6 -- didn't find any major problems, but not super confident that I didn't miss anything
fjahr:
Code review ACK f19fdd4
ryanofsky:
Code review ACK f19fdd47a6. Only change since last review is constructor cleanup (no change in behavior). I think the suggestions here from ajtowns and others are good, but shouldn't delay merging the PR (and hold up assumeutxo)
Tree-SHA512: fffb7847fb6993dd4a1a41cf11179b211b0b20b7eb5f7cf6266442136bfe9d43b830bbefcafd475bfd4af273f5573500594aa41fff03e0ed5c2a1e8562ff9269
The usage of pragmas within the macOS SDK requires LLVM Clang 8. This is
the version as our prebuilt Clang, however the minimum is worth noting here
as they may diverge and/or expert users might expect they could use an
earlier version.
If you compile using Clang 7 you'll see output like:
```bash
In file included from kernel/qcore_mac_objc.mm:44:
In file included from /bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSText.h:9:
In file included from /bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSView.h:19:
In file included from /bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSResponder.h:10:
/bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/System/Library/Frameworks/AppKit.framework/Headers/NSEvent.h:19:1: error:
expected 'push' or 'pop' after '#pragma clang attribute'
/bitcoin/depends/SDKs/Xcode-11.3.1-11C505-extracted-SDK-with-libcxx-headers/usr/include/os/availability.h:104:273: note: expanded from macro
'API_UNAVAILABLE_BEGIN'
...__API_UNAVAILABLE_BEGIN5, __API_UNAVAILABLE_BEGIN4, __API_UNAVAILABLE_BEGIN3, __API_UNAVAILABLE_BEGIN2, __API_UNAVAILABLE_BEGIN1, 0)(__VA_A...
^
fatal error: too many errors emitted, stopping now [-ferror-limit=]
20 errors generated.
```
fae656144e travis: Re-enable s390x (MarcoFalke)
Pull request description:
According to travis, the issue has been solved. Quote
> I would like to confirm that we have resolved this issue and most of our users are reported that this issue has been resolved on their end as well. Could you please re-check and see if that still exists for you?
ACKs for top commit:
theStack:
ACK fae656144e
Tree-SHA512: cf42f96d25474a9dcf0817a049e30e29714731d708f73c40a3042b0c70a71ff08f07dd96a89f0dcd5a50a63a355cf30b3511172a32b8af7d5a2e13ad222a4b49
Rather than just using it on Linux and NetBSD, use `fdatasync()` based
on whether it's available. i.e it is available in newer versions
of FreeBSD (11.1 and later).
This also aligns our code more closely with what is being done in leveldb.
Was pointed out by Luke in #19430.
Closes: #19559
While #19559 has been fixed upstream, it makes sense to not only
recommend using `CC_FOR_BUILD`here until the fix is pulled in as
part of our next libsecp update, but after discussing with Cory,
he suggested we should be setting this on OpenBSD (which still has
the an ancient GCC) regardless.
fdf697fe75 ci: Increase CCACHE_SIZE in some builds on Travis (Hennadii Stepanov)
Pull request description:
This PR will decrease build time for the longest Travis builds.
Up to ~20 min improvement with warmed up cache is expected.
Top commit has no ACKs.
Tree-SHA512: ee3a22162d03537be11e60b53de043247f6a65e35e630e0807b758a846b7e05ef2059d18846644aafa3cd5dc7d3e3f56a8ccb94cc71b95227debc9201d3142a2
5962522fbc depends: bump native_cctools for fixed lto with external clang (Cory Fields)
00d1ba7aaa depends: enable lto support for Apple's ld64 (Cory Fields)
Pull request description:
This didn't work for a few reasons (various toolchain compatibility issues) the last time I tested it, but after the last round of bumps it works with no apparent issues.
Note that this does not _enable_ LTO by default in any way, only hooks up the machinery for ```-flto``` to work correctly when specified.
Lines were split for an easier rebase after #17919 is merged.
ACKs for top commit:
fanquake:
ACK 5962522fbc. The relevant option upstream is [here](https://github.com/tpoechtrager/cctools-port/blob/master/cctools/m4/llvm.m4#L4).
Tree-SHA512: df2775e74e7bc847e6cef94cb8457d503d6c9e2fdea861e51386fa6ed5a7ba688241db3685561ae1a32f66724c1b3801727252025f00c04b90a3bdc8a4f6f93b
a8865f8b72 [net processing] Tidy up Misbehaving() (John Newbery)
d15b3afb4c [net processing] Always supply debug message to Misbehaving() (John Newbery)
634144a1c2 [net processing] Fixup MaybeDiscourageAndDisconnect() style (John Newbery)
Pull request description:
This PR makes a few minor clean-ups to `Misbehaving()` in preparation to move it out of the cs_main lock.
There are very minor logging changes but otherwise no functional changes.
ACKs for top commit:
troygiorshev:
tACK a8865f8b72
jonatack:
ACK a8865f8
fjahr:
Code review ACK a8865f8b72
promag:
Code review ACK a8865f8b72.
Tree-SHA512: 98fb4f5f76399715545a1ea19290dcebfc8cb4eff72a1d3555dd3de6e184040bb8668c9651dab21db0dfd8e674e53a5977105ef76547146c9f6fa6b4b9d2ba59
fa5979d12f rpc: Avoid useless mempool query in gettxoutproof (MarcoFalke)
fa1f7f28cb rpc: Style fixups in gettxoutproof (MarcoFalke)
Pull request description:
`GetTransaction` implicitly and unconditionally asks the mempool global for a transaction. This is problematic for several reasons:
* `gettxoutproof` is for on-chain txs only and asking the mempool for on-chain txs is confusing and minimally wasteful
* Globals are confusing and make code harder to test with unit tests
Fix both issues by passing in an optional mempool. This also helps with #19556
ACKs for top commit:
hebasto:
re-ACK fa5979d12f
jnewbery:
utACK fa5979d12f
promag:
Code review ACK fa5979d12f.
Tree-SHA512: 048361b82abfcc40481181bd44f70cfc9e97d5d6356549df34bbe30b9de7a0a72d2207a3ad0279b21f06293509b284d8967f58ca7e716263a22b20aa4e7f9c54
Check that sections are appropriately separated in virtual memory,
based on their (expected) permissions. This checks for missing
-Wl,-z,separate-code and potentially other problems.
Co-authored-by: fanquake <fanquake@gmail.com>
This flag was added to binutils/ld in the 2.30 release,
see commit c11c786f0b45617bb8807ab6a57220d5ff50e414:
> The new "-z separate-code" option will generate separate code LOAD
segment which must be in wholly disjoint pages from any other data.
It was made the default for Linux/x86 targets in the 2.31 release, see commit
f6aec96dce1ddbd8961a3aa8a2925db2021719bb:
> This patch adds --enable-separate-code to ld configure to turn on
-z separate-code by default and enables it by default for Linux/x86.
This avoids mixing code pages with data to improve cache performance
as well as security.
> To reduce x86-64 executable and shared object sizes, the maximum page
size is reduced from 2MB to 4KB when -z separate-code is turned on by
default. Note: -z max-page-size= can be used to set the maximum page
size.
> We compared SPEC CPU 2017 performance before and after this change on
Skylake server. There are no any significant performance changes.
Everything is mostly below +/-1%.
Support was also added to LLVMs lld: https://reviews.llvm.org/D64903, however
there is remains off by default.
There were concerns about an increase in binary size, however in our case, the
increase (1 page worth of bytes) would seem negligible, given we are shipping a
multi-megabyte binary, which then downloads 100's of GBs of data.
Also note that most recent versions of distros are shipping a new enough version
of binutils that this is available and/or on by default (assuming the distro has
not turned it off, I haven't checked everywhere):
CentOS 8: 2.30
Debian Buster 2.31.1
Fedora 29: 2.31.1
FreeBSD: 2.33
GNU Guix: 2.33 / 2.34
Ubuntu 18.04: 2.30
Related threads / discussion:
https://bugzilla.redhat.com/show_bug.cgi?id=1623218
Before this change, we would analyze the contents of `CNetAddr::ip[16]`
in order to tell which type is an address. Change this by introducing a
new member `CNetAddr::m_net` that explicitly tells the type of the
address.
This is necessary because in BIP155 we will not be able to tell the
address type by just looking at its raw representation (e.g. both TORv3
and I2P are "seemingly random" 32 bytes).
As a side effect of this change we no longer need to store IPv4
addresses encoded as IPv6 addresses - we can store them in proper 4
bytes (will be done in a separate commit). Also the code gets
somewhat simplified - instead of
`memcmp(ip, pchIPv4, sizeof(pchIPv4)) == 0` we can use
`m_net == NET_IPV4`.
Co-authored-by: Carl Dong <contact@carldong.me>
7b3851e947 refactor: Drop unused CBufferedFile::Seek() (Hennadii Stepanov)
Pull request description:
ACKs for top commit:
practicalswift:
ACK 7b3851e947 -- deleted code is better than unused untested code:)
MarcoFalke:
ACK 7b3851e947, assuming that removing this should either be correct or result in a compile failure
jonasschnelli:
utACK 7b3851e947
promag:
Code review ACK 7b3851e947.
Tree-SHA512: 7bfd172aa4bbe349855c1303fd9cd58093d66833fefe46bd29081bfcca4ab434b84c6b84e76e94d06b8749a5abe1dc1e184f5189136cd1403d0e5bc25ad6d456
This commit clarifies the intended usage of message_count and
last_message. Additionally it changes the only usage of message_count
to using last_message instead, bringing the code further along the
intended usage.
fa2ae0ac8d span: Add Span::empty() and use it in script/descriptor (MarcoFalke)
fa8a992589 Work around memory-aliasing in descriptor ParsePubkey (MarcoFalke)
Pull request description:
While this is not undefined behaviour, the memory aliasing trick is confusing when reading the code. Having `a.size()==0` and then access `a[0]` works in this particular case, but should probably be avoided to harden the code for the future.
ACKs for top commit:
theStack:
re-ACK fa2ae0ac8d
elichai:
ACK fa2ae0ac8d
jonatack:
ACK fa2ae0ac8d
Tree-SHA512: 0ec7b09eef45504973a195923cdf1aa8522117c8e2f69b453e5ce9aa8a7e327c71138518022c32d05133dc99cb861101ed0f60fa891814ee3e9dab3a6fa61a84
Checks that the RPC decodepsbt calculates the fee correctly, in particular for
PSBTs with segwit inputs that have both a witness- and a non-witness-UTXO type
set. Before commit 75122780e2 ("Increment input
value sum only once per UTXO in decodepsbt") the values for those inputs were
double counted.
- Make const things const.
- Replace conditional return with assert.
- Don't log the peer's IP address.
- Log the name Misbehaving directly instead of relying on __func__.
Misbehaving() could optionally take a debug string for printing to the
log file. Make this mandatory and always provide the string.
A couple of additional minor changes:
- remove the unnecessary forward declaration of Misbehaving()
- don't include the nodeid or newline in the passed debug message.
Misbehaving() adds these itself.
655b195747 [net processing] Continue SendMessages processing if not disconnecting peer (John Newbery)
a49781e56d [net processing] Only call MaybeDiscourageAndDisconnect from SendMessages (John Newbery)
a1d5a428a2 [net processing] Fix bad indentation in SendMessages() (John Newbery)
1a1c23f8d4 [net processing] Change cs_main TRY_LOCK to LOCK in SendMessages() (John Newbery)
Pull request description:
The motivation for this PR is to reduce the scope of cs_main locking in misbehavior logic. It is the first set of commits from a larger branch to move the misbehavior data out of CNodeState and into a new struct that doesn't take cs_main.
There are some very minor behavior changes in this branch, such as:
- Not checking for discouragement/disconnect in `ProcessMessages()` (and instead relying on the following check in `SendMessages()`)
- Checking for discouragement/disconnect as the first action in `SendMessages()` (and not doing ping message sending first)
- Continuing through `SendMessages()` if `MaybeDiscourageAndDisconnect()` doesn't disconnect the peer (rather than dropping out of `SendMessages()`
ACKs for top commit:
jonatack:
re-ACK 655b195 per `git range-diff 505b4ed f54af5e 655b195`, code/commit messages review, a bit of code history, and debug build.
MarcoFalke:
ACK 655b195747 only some style-nits 🚁
promag:
Code review ACK 655b195747.
ariard:
Code Review ACK 655b195
Tree-SHA512: fd6d7bc6bb789f5fb7771fb6a45f61a8faba32af93b766554f562144f9631d15c9cc849a383e71743ef73e610b4ee14853666f6fbf08a3ae35176d48c76c65d3
ef3d4ce4c3 build: call AC_PATH_TOOL for dsymutil in macOS cross-compile (fanquake)
Pull request description:
While testing #19530 I noticed that we couldn't call [`dsymutil`](https://www.llvm.org/docs/CommandGuide/dsymutil.html) after LTO:
```bash
../libtool: line 10643: x86_64-apple-darwin16-dsymutil: command not found
```
This updates configure to call `AC_PATH_TOOL` so that we end up with the
full path to dsymutil, similar to `otool` and `install_name_tool`, ie:
`/bitcoin/depends/x86_64-apple-darwin16/share/../native/bin/x86_64-apple-darwin16-dsymutil`.
ACKs for top commit:
laanwj:
Code review ACK ef3d4ce4c3
theuni:
ACK ef3d4ce4c3.
Tree-SHA512: e4fa93e7f9f7945289143dfe2a6645ad8ee7f3bee0793412b3509901a30566d6f952e3b39e0e525a54f8dbd0c480f8da70fc6cb80b07800d11b0c6071fbb7466
9c69cfe4c5 Add <datadir>/settings.json persistent settings storage. (Russell Yanofsky)
eb682c5700 util: Add ReadSettings and WriteSettings functions (Russell Yanofsky)
Pull request description:
Persistent settings are used in followup PRs #15936 to unify gui settings between bitcoin-qt and bitcoind, and #15937 to add a load_on_startup flag to the loadwallet RPC and maintain a dynamic list of wallets that should be loaded on startup that also can be shared between bitcoind and bitcoin-qt.
ACKs for top commit:
MarcoFalke:
Approach re-ACK 9c69cfe4c5🌾
jnewbery:
utACK 9c69cfe4c5
Tree-SHA512: 39fcc6051717117c9141e934de1d0d3f739484be4685cdf97d54de967c8c816502b4fd0de12114433beaa5c5b7060c810fd8ae4e2b3ce7c371eb729ac01ba2e1
2aac093a3d test: Add test coverage for -networkactive option (Hennadii Stepanov)
3c58129b12 net: Log network activity status change unconditionally (Hennadii Stepanov)
62fe6aa87e net: Add -networkactive option (Hennadii Stepanov)
Pull request description:
Some Bitcoin Core activity is completely local (offline), e.g., reindexing.
The `setnetworkactive` RPC command is already present. This PR adds the corresponding command-line argument / config option, and allows to start the client with disabled p2p network by providing `-networkactive=0` or `-nonetworkactive`.
This was done while reviewing #16981.
ACKs for top commit:
MarcoFalke:
re-ACK 2aac093a3d🏠
LarryRuane:
ACK 2aac093a3d
Tree-SHA512: 446d791b46d7b556d7694df7b1f88cd4fbc09301fe4eaf036b45cb8166ed806156353cc03788a07b633d5887d5eee30a7c02a2d4307141c8ccc75e0a88145636
d416ae560e walletdb: Introduce WalletDatabase abstract class (Andrew Chow)
2179dbcbcd walletdb: Add BerkeleyDatabase::Open dummy function (Andrew Chow)
71d28e7cdc walletdb: Introduce AddRef and RemoveRef functions (Andrew Chow)
27b2766384 walletdb: Move BerkeleyDatabase::Flush(true) to Close() (Andrew Chow)
Pull request description:
A `WalletDatabase` abstract class is created from `BerkeleyDatabase` and is implemented by `BerkeleyDatabase`. First, to get to the point that this is possible, 4 functions need to be added to `BerkeleyDatabase`: `AddRef`, `RemoveRef`, `Open`, and `Close`.
First the increment and decrement of `mapFileUseCount` is refactored into separate functions `AddRef` and `RemoveRef`.
`Open` is introduced as a dummy function. This will raise an exception so that it always fails.
`Close` is refactored from `Flush`. The `shutdown` argument in `Flush` is removed and instead `Flush(true)` is now the `Close` function.
Split from #18971
Requires #19325
ACKs for top commit:
ryanofsky:
Code review ACK d416ae560e. Only changes since last review were rebasing after base PR #19334 merge, and adding cs_db lock in BerkeleyDatabase destructor, which should avoid races accessing env->m_databases and env->m_fileids
fjahr:
Code review ACK d416ae560e
meshcollider:
Code review & test run ACK d416ae560e
Tree-SHA512: 98d05ec093d7446c4488e2b0914584222a331e9a2f4d5be6af98e3f6d78fdd8e75526c12f91a8a52d4820c25bce02aa02aabe92d38bee7eb2fce07d0691b7b0d
The `setnetworkactive' RPC command is already present.
This new option allows to start the client with disabled p2p network
activity for testing or reindexing.
