a50e0b1bcb120e097a2b23dbfb71533b2f9c4d6c qt, refactor: Add `transactionoverviewwidget.cpp` source file (Hennadii Stepanov)
Pull request description:
The `TransactionOverviewWidget` class was added in bitcoin-core/gui#176 as a header-only one.
Apparently, in upcoming [CMake project](https://github.com/hebasto/bitcoin/pull/3), CMake [AUTOMOC](https://cmake.org/cmake/help/latest/prop_tgt/AUTOMOC.html) could be integrated better/simpler, if `QObject`-derived class implementation been placed into a source file.
From our [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#source-code-organization):
> Implementation code should go into the `.cpp` file and not the `.h`, unless necessary due to template usage or when performance due to inlining is critical.
ACKs for top commit:
Sjors:
tACK a50e0b1bcb120e097a2b23dbfb71533b2f9c4d6c
shaavan:
ACK a50e0b1bcb120e097a2b23dbfb71533b2f9c4d6c
Tree-SHA512: 4707b6be1c5e794c4014475f826ac45ec833e472db11f12d29995f9c5a599ee98622ad54f0af72734b192144b626411c69acdafa0e6d1a390bdebfd7e570f377
0f1a259657280afc727db97689512aef5ca928fc miner: Make mempool optional for BlockAssembler (Carl Dong)
cc5739b27df830d138119eaa13f2286d91d0dadd miner: Make UpdatePackagesForAdded static (Carl Dong)
f024578b3a5c40e275e23d1c8e82530e235fdbf9 miner: Absorb SkipMapTxEntry into addPackageTxs (Carl Dong)
Pull request description:
This is part of the libbitcoinkernel project: #24303, https://github.com/bitcoin/bitcoin/projects/18
This is **_NOT_** dependent on, but is a "companion-PR" to #25215.
### Abstract
This PR removes the need to construct `BlockAssembler` with temporary, empty mempools in cases where we don't want to source transactions from the mempool (e.g. in `TestChain100Setup::CreateBlock` and `generateblock`). After this PR, `BlockAssembler` will accept a `CTxMemPool` pointer and handle the `nullptr` case instead of requiring a `CTxMemPool` reference.
An overview of the changes is best seen in the changes in the header file:
```diff
diff --git a/src/node/miner.h b/src/node/miner.h
index 7cf8e3fb9e..7e9f503602 100644
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -147,7 +147,7 @@ private:
int64_t m_lock_time_cutoff;
const CChainParams& chainparams;
- const CTxMemPool& m_mempool;
+ const CTxMemPool* m_mempool;
CChainState& m_chainstate;
public:
@@ -157,8 +157,8 @@ public:
CFeeRate blockMinFeeRate;
};
- explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool);
- explicit BlockAssembler(CChainState& chainstate, const CTxMemPool& mempool, const Options& options);
+ explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool);
+ explicit BlockAssembler(CChainState& chainstate, const CTxMemPool* mempool, const Options& options);
/** Construct a new block template with coinbase to scriptPubKeyIn */
std::unique_ptr<CBlockTemplate> CreateNewBlock(const CScript& scriptPubKeyIn);
@@ -177,7 +177,7 @@ private:
/** Add transactions based on feerate including unconfirmed ancestors
* Increments nPackagesSelected / nDescendantsUpdated with corresponding
* statistics from the package selection (for logging statistics). */
- void addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
+ void addPackageTxs(const CTxMemPool& mempool, int& nPackagesSelected, int& nDescendantsUpdated) EXCLUSIVE_LOCKS_REQUIRED(mempool.cs);
// helper functions for addPackageTxs()
/** Remove confirmed (inBlock) entries from given set */
@@ -189,15 +189,8 @@ private:
* These checks should always succeed, and they're here
* only as an extra check in case of suboptimal node configuration */
bool TestPackageTransactions(const CTxMemPool::setEntries& package) const;
- /** Return true if given transaction from mapTx has already been evaluated,
- * or if the transaction's cached data in mapTx is incorrect. */
- bool SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set& mapModifiedTx, CTxMemPool::setEntries& failedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
/** Sort the package in an order that is valid to appear in a block */
void SortForBlock(const CTxMemPool::setEntries& package, std::vector<CTxMemPool::txiter>& sortedEntries);
- /** Add descendants of given transactions to mapModifiedTx with ancestor
- * state updated assuming given transactions are inBlock. Returns number
- * of updated descendants. */
- int UpdatePackagesForAdded(const CTxMemPool::setEntries& alreadyAdded, indexed_modified_transaction_set& mapModifiedTx) EXCLUSIVE_LOCKS_REQUIRED(m_mempool.cs);
};
int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
```
### Alternatives
Aside from approach in this current PR, we can also take the approach of moving the `CTxMemPool*` argument from the `BlockAssembler` constructor to `BlockAssembler::CreateNewBlock`, since that's where it's needed anyway. I did not push this approach because it requires quite a lot of call sites to be changed. However, I do have it coded up and can do that if people express a strong preference. This would look something like:
```
BlockAssembler::BlockAssembler(CChainState& chainstate, const Options& options);
BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, const CTxMemPool* maybe_mempool);
```
### Future work
Although wholly out of scope for this PR, we could potentially refine the `BlockAssembler` interface further, so that we have:
```
BlockAssembler::BlockAssembler(CChainState& chainstate, const Options& options);
BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, std::vector<CTransaction>& txs);
BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, const CTxMemPool& mempool);
```
Whereby `TestChain100Setup::CreateBlock` and `generateblock` would call the `BlockAssembler::CreateNewBlock` that takes in `CTransaction`s and we can potentially remove `RegenerateCommitments` altogether. All other callers can use the `CTxMemPool` version.
ACKs for top commit:
glozow:
ACK 0f1a259657280afc727db97689512aef5ca928fc
laanwj:
Code review ACK 0f1a259657280afc727db97689512aef5ca928fc
MarcoFalke:
ACK 0f1a259657280afc727db97689512aef5ca928fc 🐊
Tree-SHA512: 2b4b1dbb43d85719f241ad1f19ceb7fc50cf764721da425a3d1ff71bd16328c4f86acff22e565bc9abee770d3ac8827a6676b66daa93dbf42dd817ad929e9448
fafddafc2c60c7a176c5367967181cc1cabf3196 refactor: Introduce PeerManagerImpl::RejectIncomingTxs (MacroFake)
Pull request description:
Currently there are some confusions in net_processing:
* There is confusion between `-blocksonly mode` and `block-relay-only`, so adjust all comments to use the same nomenclature.
* Whether to disconnect peers for providing invs/txs is implemented differently. For example, it seems a bit confusing to disconnect `block-relay-only` peers with `relay` permission when they send a tx message, but not when they send an inv message. Also, keeping track of their inv announcements seems both wasteful and confusing, as it does nothing. This isn't possible in practice, as outbound connections do not have permissions assigned, but sees fragile to rely on. Especially in light of proposed changes to make that possible: https://github.com/bitcoin/bitcoin/pull/17167
ACKs for top commit:
MarcoFalke:
Should be trivial to re-ACK with `git range-diff bitcoin-core/master fa2b5fe0c1 fafddafc2c`.
jnewbery:
Code review ACK fafddafc2c
mzumsande:
ACK fafddafc2c60c7a176c5367967181cc1cabf3196
Tree-SHA512: 73bf91afe93be619169cfbf3bf80cb08a5e6f73df4e0318b86817bd4d45f67408ea85998855992281d2decc9d24f7d75cffb83a0518d670090907309df8a3490
018d70b58726b361b6951e0e6de04f13eb97a89d scripted-diff: Avoid incompatibility with CMake AUTOUIC feature (Hennadii Stepanov)
Pull request description:
Working on [migration](https://github.com/hebasto/bitcoin/pull/3) from Autotools to CMake build system, I found that our current code base needs to be adjusted.
CMake [allows](https://cmake.org/cmake/help/latest/prop_tgt/AUTOUIC.html) to
> handle the Qt `uic` code generator automatically
When using this feature, statements like `#include "ui_<ui_base>.h"` are processed in a special way.
The `node/ui_interface.h` unintentionally breaks this feature. Of course, it is possible to provide a list of source files to be excluded from `AUTOUIC`. But, unfortunately, this approach does not work for the `qt/sendcoinsdialog.cpp` source file, where there are both b71d37da2c/src/qt/sendcoinsdialog.cpp (L10) and b71d37da2c/src/qt/sendcoinsdialog.cpp (L24)
ACKs for top commit:
MarcoFalke:
cr ACK 018d70b58726b361b6951e0e6de04f13eb97a89d
ryanofsky:
Code review ACK 018d70b58726b361b6951e0e6de04f13eb97a89d
furszy:
Code review ACK 018d70b5
Tree-SHA512: 4fc83f2e5a82c8ab15c3c3d68f48b9863c47b96c0a66b6276b9b4dfc6063abffd73a16382acfe116553487b3ac697dbde2d9ada1b92010c5d8f8c6aa06f56428
ecff20db286e2f5d3afe32cfaae72de69d34d23c logging: use LogPrintfCategory rather than a manual category (Jon Atack)
eb8aab759fb15824a5dd3004e689d0eb5b884a32 logging: add LogPrintfCategory to log unconditionally with category (Jon Atack)
Pull request description:
These are the next two commits from #25203.
- Add `LogPrintfCategory` to log unconditionally while prefixing the output with the passed category name. Add documentation and a unit test, and update the `lint-logs.py` and `lint-format-strings.py` scripts.
- Replace the log messages that manually print a category, with `LogPrintfCategory`. In upcoming commits, it will likely be used in many other cases, such as to replace `LogPrintf` where it makes sense.
ACKs for top commit:
klementtan:
Code Review ACK ecff20db286e2f5d3afe32cfaae72de69d34d23c
laanwj:
Code review ACK ecff20db286e2f5d3afe32cfaae72de69d34d23c
brunoerg:
ACK ecff20db286e2f5d3afe32cfaae72de69d34d23c
Tree-SHA512: ad3a82835254f7606efcd14b88f3d9072f1eb9b25db1321ed38ef6a4ec60efd555d78f5e19d93736f2f8500251d06f8beee9d694a153f24bf5cce3590a2a45a5
1cb42aeda37f4979923cd7e1c85febe994480de6 util: modify Win32LockedPageAllocator to query windows for limit (Oskar Mendel)
Pull request description:
This PR resolves a todo within the Win32LockedPageAllocator: `// TODO is there a limit on Windows, how to get it?`.
The idea is to use the Windows API to get the limits like the posix based allocator does with `getrlimit`.
I use [GetProcessWorkingSetSize](https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-getprocessworkingsetsize) to perform this task and fallback to `return std::numeric_limits<size_t>::max();` just like the posix implementation does.
ACKs for top commit:
sipsorcery:
tACK 1cb42aeda37f4979923cd7e1c85febe994480de6.
Tree-SHA512: 7bdd8a57a4e64ee59d752417a519656e03526878462060753be4dce481eff4889fb5edc1bdbd575b707d9b2dfe255c87da9ef67baac97de9ac5e70a04c852081
Currently there are some confusions in net_processing:
* There is confusion between `-blocksonly mode` and `block-relay-only`,
so adjust all comments to use the same nomenclature.
* Whether to disconnect peers for providing invs/txs is implemented
differently. For example, it seems a bit confusing to disconnect
`block-relay-only` peers with `relay` permission when they send a tx
message, but not when they send an inv message. Also, keeping track of
their inv announcements seems both wasteful and confusing, as it does
nothing. This isn't possible in practice, as outbound connections do
not have permissions assigned, but sees fragile to rely on. Especially
in light of proposed changes to make that possible:
https://github.com/bitcoin/bitcoin/pull/17167
e47c6c76561807d30cff3c2e5372ea83c91a3677 Reset settings.json when GUI options are reset (Ryan Ofsky)
99ccc02b652cf67a3aec66371fcb6bbe737571a7 Add release notes about unified bitcoin-qt and bitcoind persistent settings (Ryan Ofsky)
504b06b1dec9d9329c83b13c7c36ca710ebcd349 Migrate -lang setting from QSettings to settings.json (Ryan Ofsky)
9a016a3c07d4becf0651ef58c7160180c5f25a0c Migrate -prune setting from QSettings to settings.json (Ryan Ofsky)
f067e1943361b7bfa78a423528759e9edffa7482 Migrate -proxy and -onion settings from QSettings to settings.json (Ryan Ofsky)
a09e3b7cf29c3b1fd320badbed32275e0aa83cda Migrate -listen and -server settings from QSettings to settings.json (Ryan Ofsky)
d2ada6e63583cad91e92b49c4dbf8c7ff086a758 Migrate -upnp and -natpmp settings from QSettings to settings.json (Ryan Ofsky)
1dc4fc29c1086420b7dc51b20c0b7a18fecb4462 Migrate -spendzeroconfchange and -signer settings from QSettings to settings.json (Ryan Ofsky)
a7ef6d5975a5f40b90b2709b32a00647bd2bd5a3 Migrate -par setting from QSettings to settings.json (Ryan Ofsky)
284f339de68905131331f7fdb4c0b945c9a1b8cd Migrate -dbcache setting from QSettings to settings.json (Ryan Ofsky)
Pull request description:
If a setting like pruning, port mapping, or a network proxy is enabled in the GUI, it will now be stored in the bitcoin persistent setting file in the datadir and shared with bitcoind, instead of being stored as Qt settings which end up in the the windows registry or platform specific config files and are ignored by bitcoind.
This PR has been split off from bitcoin/bitcoin#15936 so some review of these commits previously took place in that PR.
ACKs for top commit:
furszy:
Code review ACK e47c6c76
hebasto:
ACK e47c6c76561807d30cff3c2e5372ea83c91a3677
Tree-SHA512: 076ea7c7efe67805b4a357113bfe1643dce364d0032774106de59566a0ed5771d57a5923920085e03d686beb34b98114bd278555dfdf8bb7af0b778b0f35b7d2
1f653dc2623ee2be3e1eeeaf5ce0e97966cecd6a qt, wallet, refactor: Make `WalletModel::sendCoins()` return `void` (Hennadii Stepanov)
Pull request description:
Currently, the `WalletModel::sendCoins()` function always returns the same value.
Also dead and noop (calling `processSendCoinsReturn(OK)`) code has been removed.
The other `return` statements have been removed from the `WalletModel::sendCoins()` function in bitcoin/bitcoin#17154 and bitcoin/bitcoin#17165.
ACKs for top commit:
kristapsk:
cr ACK 1f653dc2623ee2be3e1eeeaf5ce0e97966cecd6a
furszy:
Code review ACK 1f653dc2
shaavan:
Code Review ACK 1f653dc2623ee2be3e1eeeaf5ce0e97966cecd6a
w0xlt:
Code Review ACK 1f653dc262
Tree-SHA512: 2b59495a7fc10b4de30fcc63fc3af92d50406e16031112eb72494736dce193ac1fbac0802623496cf81edcd16766e1647d9c4f3a607b3eb84cc50e273b999c04
44c2452fd Merge bitcoin-core/secp256k1#1105: Don't export symbols in static libraries
6f6cab998 abi: Don't export symbols in static Windows libraries
485f608fa Merge bitcoin-core/secp256k1#1104: Fix the false positive of `SECP_64BIT_ASM_CHECK`
8b013fce5 Merge bitcoin-core/secp256k1#1056: Save negations in var-time group addition
7efc9835a Fix the false positive of `SECP_64BIT_ASM_CHECK`
2f984ffc4 Save negations in var-time group addition
git-subtree-dir: src/secp256k1
git-subtree-split: 44c2452fd387f7ca604ab42d73746e7d3a44d8a2
Achieve this by adding a MAIN_FUNCTION macro, consolidating the docs, and
introducing the macro across our distributed binaries.
Also update the docs to explain that anyone using binutils < 2.36 is
effected by this issue. Release builds are not, because they use binutils
2.37. Currently LTS Linux distros, like Ubuntu Focal, ship with 2.34.
https://packages.ubuntu.com/focal/binutils
ce893c0497fc9b8ab9752153dfcc77c9f427545e doc: Update developer notes (Anthony Towns)
d2852917eecad6ab422a7b2c9892d351a7f0cc96 sync.h: Imply negative assertions when calling LOCK (Anthony Towns)
bba87c0553780eacf0317fbfec7330ea27aa02f8 scripted-diff: Convert global Mutexes to GlobalMutexes (Anthony Towns)
a559509a0b8cade27199740212d7b589f71a0e3b sync.h: Add GlobalMutex type (Anthony Towns)
be6aa72f9f8d50b6b5b19b319a74abe7ab4099ff qt/clientmodel: thread safety annotation for m_cached_tip_mutex (Anthony Towns)
f24bd45b37e1b2d19e5a053dbfefa30306c1d41a net_processing: thread safety annotation for m_tx_relay_mutex (Anthony Towns)
Pull request description:
This changes `LOCK(mutex)` for non-global, non-recursive mutexes to be annotated with the negative capability for the mutex it refers to, to prevent . clang applies negative capabilities recursively, so this helps avoid forgetting to annotate functions.
This can't reasonably be used for globals, because clang would require every function to be annotated with `EXCLUSIVE_LOCKS_REQUIRED(!g_mutex)` for each global mutex; so this introduces a trivial `GlobalMutex` subclass of `Mutex`, and reduces the annotations for both `GlobalMutex` to `LOCKS_EXCLUDED` which only catches trivial errors (eg (`LOCK(x); LOCK(x);`).
ACKs for top commit:
MarcoFalke:
review ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e 🐦
hebasto:
ACK ce893c0497fc9b8ab9752153dfcc77c9f427545e
Tree-SHA512: 5c35e8c7677ce3d994a7e3774f4344adad496223a51b3a1d1d3b5f20684b2e1d5cff688eb3fbc8d33e1b9940dfa76e515f9434e21de6f3ce3c935e29a319f529
Code introduced in #15649 added usage of `timingsafe_bcmp()`, if
available, otherwise falling back to our own implementation. However
the relevant build system check was never added, so currently, we'll
always just use our implementation, as HAVE_TIMINGSAFE_BCMP will never
be defined.
Add the check for timingsafe_bcmp. Note that as far as I'm aware, it's
only available on OpenBSD.
d575413fb8f8569803ed0050c15be003ef50951c doc: add `desig` to ignore-words (brunoerg)
c06cc41ddbac3611d5c7826a2a3f2cbae9aeea12 doc: fix typo in kernel/context.h (brunoerg)
Pull request description:
This PR fixes a typo in `kernel/context.h` (libary => library) and add `desig` to ignore-words since it's a valid word, see:
b9416c3847/src/net.cpp (L1105-L1117)
ACKs for top commit:
fanquake:
ACK d575413fb8f8569803ed0050c15be003ef50951c
Tree-SHA512: 2d548c737b8184d0243445c7503f3f68256ecb0970bd834d52de099de3cd8c8b9c140e2b77d55e2542fbd45b1d21cbdee639f5b2ef8138c37b8b72e5211029c3
8888bd43c100f9f0ca1122fcc896fb7b999d61c6 Remove redundant nLastTry check (MarcoFalke)
00001e57fe74c061aa9cbc72b07252335cb566e0 Remove redundant nTime checks (MarcoFalke)
Pull request description:
Split out from https://github.com/bitcoin/bitcoin/pull/24697 because it makes sense on its own.
ACKs for top commit:
dergoegge:
re-ACK 8888bd43c100f9f0ca1122fcc896fb7b999d61c6
naumenkogs:
utACK 8888bd43c100f9f0ca1122fcc896fb7b999d61c6
Tree-SHA512: 32c6cde1c71e943c76b7991c2c24caf29ae467ab4ea2d758483a0cee64625190d1a833b468e8eab1f834beeb2c365af96552c14b05270f08cf63790e0707581d
Here we update only the log messages that manually print a category.
In upcoming commits, LogPrintCategory will likely be used in many
other cases, such as to replace `LogPrintf` where it makes sense.
292828cd7744ec7eadede4ad54aa2117087c5435 [test] Test addr cache for multiple onion binds (dergoegge)
3382905befd23364989d941038bf7b1530fea0dc [net] Seed addr cache randomizer with port from binding address (dergoegge)
f10e80b6e4fbc151abbf1c20fbdcc3581d3688f0 [net] Use ConnectedThroughNetwork() instead of GetNetwork() to seed addr cache randomizer (dergoegge)
Pull request description:
The addr cache id randomizer is currently supposed to be seeded with the network of the inbound connection and the local socket (only the address is used not the port): a8098f2cef/src/net.cpp (L2800-L2804)
For inbound onion connections `CNode::addr.GetNetwork()` returns `NET_UNROUTABLE` and `CNode::addrBind` is set to `127.0.0.1:<onion bind port>`. This results in the same addr cache for all inbound connections on 127.0.0.1 binds.
To avoid the same addr cache across all onion and other 127.0.0.1 binds, we should seed the addr cache randomizer with the correct network for inbound onion connections (using `CNode::ConnectedThroughNetwork()`) as well as the port of `CNode::addrBind`.
ACKs for top commit:
sipa:
utACK 292828cd7744ec7eadede4ad54aa2117087c5435
mzumsande:
Code Review ACK 292828cd7744ec7eadede4ad54aa2117087c5435
naumenkogs:
utACK 292828cd7744ec7eadede4ad54aa2117087c5435
Tree-SHA512: d0be13bab6bc121c2926d4b168687f6c2ed4ce0c9dd19be71eb4886adeba8afc3daacdc4e232a0ba3b03a89d69b618abc5595b69abd1ad0c476d825bc6ea1f9f
57fb37c27599fc865f20b42a27bb9c227f384de3 wallet: CommitTransaction, remove extra wtx lookup and add exception for a possible db write error. (furszy)
Pull request description:
Two points for `CWallet::CommitTransaction`:
1) The extra wtx lookup:
As we are calling to `AddToWallet` first, which returns the recently added/updated wtx pointer, there is no need to look up the wtx again few lines later. We can just use it.
2) The db write error:
`AddToWallet` can only return a nullptr if the db write fails, which inside `CommitTransaction` translates to an exception throw cause. We expect everywhere that `CommitTransaction` always succeed.
------------------------------------------------
Extra note:
This finding generated another working path for me :)
It starts with the following question: why are we returning a nullptr from `AddToWallet` if the db write failed without removing the recently added transaction from the wallet's map?..
Can led to a wallet invalid state where the inputs of this new transaction are not marked dirty, while the transaction that spends them still exist on the in-memory wallet tx map.
-- I'm writing it here to gather some feedback first and not forget it, will create a follow-up PR in the coming days 🚜 --
ACKs for top commit:
achow101:
ACK 57fb37c27599fc865f20b42a27bb9c227f384de3
jonatack:
ACK 57fb37c
ryanofsky:
Code review ACK 57fb37c27599fc865f20b42a27bb9c227f384de3. Seems like a clear improvement. Better to fail earlier with a better error message if the failure is going to happen anyway
Tree-SHA512: 80e59c01852cfbbc70a5de1a1c2c59b5e572f9eaa08c2175112cb515256e63fa04c7942f92a513b620d6b06e66392029ebe8902287c456efdbee58a7a5ae42da
d40550d725fa7afd6916510164bbaa95e3ec0a5f scripted-diff: remove duplicate categories from LogPrint output (Jon Atack)
Pull request description:
This is the first commit from #25203.
- Scripted-diff: de-duplicate logging category output for the tor, i2p, net, zmq, and prune messages (e.g. where I found duplicates), as these category prefixes are now printed automatically since #24464
examples before
```
[tor] tor: Successfully connected!
[i2p] I2P: Creating SAM session with 127.0.0.1:7656
[zmq] zmq: Initialize notification interface
[net] net: enabling extra block-relay-only peers
```
after
```
[tor] Successfully connected!
[i2p] Creating SAM session with 127.0.0.1:7656
[zmq] Initialize notification interface
[net] enabling extra block-relay-only peers
```
ACKs for top commit:
klementtan:
crACK d40550d725fa7afd6916510164bbaa95e3ec0a5f
MarcoFalke:
cr ACK d40550d725fa7afd6916510164bbaa95e3ec0a5f
Tree-SHA512: 63b799f2f899f0597981dd1acb91ef4439cd00b257a9eb19d67c4ce2c4dc72a95ac5761cb78f2a19090a10be74f23ea1db6929ed942ba0d008b4be563f0d5e7e
fa4068b4e2192f168bb120624eca5735f0dadf6f Move minRelayTxFee to policy/settings (MacroFake)
Pull request description:
Seems a bit confusing to put policy stuff into validation, so fix that.
Also fix includes via `iwyu`.
ACKs for top commit:
ariard:
ACK fa4068b, the includes move compiles well locally.
ryanofsky:
Code review ACK fa4068b4e2192f168bb120624eca5735f0dadf6f. Make sense to move the global variable to policy/settings and the default constant to policy/policy. Ariard points out other constants that could be moved, which seems fine, but it seems like moving the global variable to be with other related global variables is more significant.
Tree-SHA512: adf9619002610d1877f3aef0a9e6115fc4c2ad64135a3e5100824c650b560c47f47ac28894c6214a50a7888355252a9f6f7cec98c23a771a1964160ef1ca77de
e593ae07c4fb41a26c95dbd03301607fc5b4d5e2 Bugfix: RPC/blockchain: pruneblockchain: Return the height of the actual last pruned block (Luke Dashjr)
Pull request description:
From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last block pruned was returned, subject to a bug if there were blocks left unpruned due to sharing files with later blocks.
In #15991, this was "fixed" to the current implementation, introducing a new bug: now, it returns the first *unpruned* block.
Since the user provides the parameter as a block to include in pruning, it makes more sense to fix the behaviour to match the documentation.
~~(Additionally, the description of "pruneheight" in getblockchaininfo is fixed to be technically correct)~~
ACKs for top commit:
fjahr:
utACK e593ae07c4fb41a26c95dbd03301607fc5b4d5e2
ryanofsky:
Code review ACK e593ae07c4fb41a26c95dbd03301607fc5b4d5e2. Just rebased since last review. Maybe some of the original reviewers of #15991 will want to take a look at this to correct the mistake that was introduced there!
Tree-SHA512: c2d511df80682d57260aae8af1665f9d7eaed16448f185f4c9f23c78fa9b8289a02053da7a0b83643fef57610d601ea63b59ff39661a51f4827f1eb27cc30594
...also adjust callers
Changes:
- In BlockAssembler::CreateNewBlock, we now only lock m_mempool->cs and
call addPackageTxs if m_mempool is not nullptr
- BlockAssembler::addPackageTxs now takes in a mempool reference, and is
annotated to require that mempool's lock.
- In TestChain100Setup::CreateBlock and generateblock, don't construct
an empty mempool, just pass in a nullptr for mempool
3a9b9bb38e653c8ff7220b9af6e337a90c2c22dc test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg)
eaf6f630c0190c634b5f1c85f749437f4209cc36 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg)
Pull request description:
Fixes#25127
If there are any uncompressed keys when calling `AddAndGetMultisigDestination`, it will just default to a legacy address regardless of the chosen `address_type`. So, #23113 added a warnings field which will warn the user why their address format is different.
However, when creating a multisig (p2sh-segwit), it is returning an inappropriate warning, because when getting the output type from destination (`OutputTypeFromDestination`), it returns `ScriptHash` for both legacy and `P2SH_SEGWIT`. So, since `P2SH_SEGWIT` is different from `ScriptHash`, it returns the warning:
192d639a6b/src/rpc/output_script.cpp (L166-L169)
So, to avoid this mistake I changed `OutputTypeFromDestination` to `descriptor->GetOutputType()` to get the appropriate output type.
ACKs for top commit:
jonatack:
ACK 3a9b9bb38e653c8ff7220b9af6e337a90c2c22dc
laanwj:
Code review ACK 3a9b9bb38e653c8ff7220b9af6e337a90c2c22dc
Tree-SHA512: 49f717479c2b8906277e7591ddd4747f7961c2d5c77494b5124045de9036a4277d46b9ad99279d51f0c4484284c445f1e1d3c55c49bbf0716741bad426a89369
d1b7bcbca2be4eae3bf7572a8343a3bd69ce61a9 qt: Drop no longer supported Android architecture (Hennadii Stepanov)
Pull request description:
The `i686-linux-android` arch support has been dropped since bitcoin/bitcoin#23744.
ACKs for top commit:
katesalazar:
ACK d1b7bcbca2be4eae3bf7572a8343a3bd69ce61a9
icota:
utACK d1b7bcbca2
prusnak:
Approach ACK d1b7bcbca2be4eae3bf7572a8343a3bd69ce61a9
Tree-SHA512: 13689ec8c63c92b9a52a3c25edc35536b8e51ff583f57c45b168515f928d020d6bb85d03db9efd8d5efd57b944dfd313a89f5ff8a52f99982ccc8d9671f6e7a9
f3a50c9dfe645c548713e44e0eaf26ea9917a379 miniscript: rename IsSane and IsSaneSubexpression to prevent misuse (Antoine Poinsot)
c5fe5163dc31db939c44129f2ff8283b290a9330 miniscript: nit: don't return after assert(false) (Antoine Poinsot)
7bbaca9d8d355a17348a8d01e3e2521c5de466b0 miniscript: explicit the threshold size computation in multi() (Antoine Poinsot)
8323e4249db50d46ae4f43c1d8a50666549ae938 miniscript: add an OpCode typedef for readability (Antoine Poinsot)
7a549c6c59e6babbae76af008433426c6fa38fe2 miniscript: mark nodes with duplicate keys as insane (Antoine Poinsot)
8c0f8bf7bc3750fad648af1a548517a272114bca fuzz: add a Miniscript target for string representation roundtripping (Antoine Poinsot)
be34d5077b2fede7404de7706362f5858c443525 fuzz: rename and improve the Miniscript Script roundtrip target (Antoine Poinsot)
7eb70f0ac0a54adabc566e2b93bbf6b2beb54a79 miniscript: tiny doc fixups (Antoine Poinsot)
5cea85f12cba5dcfe3a298eddfa711f582adffac miniscript: split ValidSatisfactions from IsSane (Antoine Poinsot)
a0f064dc1474a048e236bfff12f4def3aa11daf3 miniscript: introduce a CheckTimeLocksMix helper (Antoine Poinsot)
ed45ee3882e69266d550b56ff69388e071f0ad1b miniscript: use optional instead of bool/outarg (Antoine Poinsot)
1ab8d89fd1bdb3c0f2a506b4a10df6c23ba21c48 miniscript: make equality operator non-recursive (Antoine Poinsot)
5922c662c08a061b3b3d5ac34a31f9f9d4640d47 scripted-diff: miniscript: rename 'nodetype' variables to 'fragment' (Antoine Poinsot)
c5f65db0f03b52bc4525acae944173829290ce6f miniscript: remove a workaround for a GCC 4.8 bug (Antoine Poinsot)
Pull request description:
The Miniscript repository and the Miniscript integration PR here have been a moving target for the past months, and some final cleanups were done there that were not included here. I initially intended to add some small followup commits to #24148 but i think there are enough of them to be worth a followup PR on its own.
Some parts of the code did not change since it was initially written in 2019, and the code could use some modernization. (Use std::optional instead of out args, remove old compiler workarounds).
We refactored the helpers to be more meaningful, and also did some renaming. A new fuzz target was also added and both were merged in a single file. 2 more will be added in #24149 that will be contained in this file too.
The only behaviour change in this PR is to rule out Miniscript with duplicate keys from sane Miniscripts. In a P2WSH context, signatures can be rebounded (Miniscript does not use CODESEPARATOR) and it's reasonable to assume that reusing keys across the Script drops the malleability guarantees.
It was previously assumed such Miniscript would never exist in the first place since a compiler should never create them. We finally agreed that if one were to exist (say, written by hand or from a buggy compiler) it would be very confusing if an imported Miniscript descriptor (after #24148) with duplicate keys was deemed sane (ie, "safe to use") by Bitcoin Core. We now check for duplicate keys in the constructor.
This is (still) joint work with Pieter Wuille. (Actually he entirely authored the cleanups and code modernization.)
ACKs for top commit:
sipa:
utACK f3a50c9dfe645c548713e44e0eaf26ea9917a379 (with the caveat that a lot of it is my own code)
sanket1729:
code review ACK f3a50c9dfe645c548713e44e0eaf26ea9917a379. Did not review the fuzz tests.
Tree-SHA512: c043325e4936fe25e8ece4266b46119e000c6745f88cea530fed1edf01c80f03ee6f9edc83b6e9d42ca01688d184bad16bfd967c5bb8037744e726993adf3deb
d87784ac87364fc977bbf9769c8bdb72dea8cbf9 kernel: SanityChecks: Return an error struct (Carl Dong)
265d6393bf9ef52e7ef7de97ca9c031da82a5ad1 Move init::SanityCheck to kernel::SanityCheck (Carl Dong)
fed085a1a4cd2787202752b6a0d98e42dce97f09 init: Initialize globals with kernel::Context's life (Carl Dong)
7d03feef8156ef37a4efa01dc591467bc7d957bf kernel: Introduce empty and unused kernel::Context (Carl Dong)
eeb4fc20c578b1e428a92d64cc9f8f903a677580 test: Use Set/UnsetGlobals in BasicTestingSetup (Carl Dong)
Pull request description:
The full `init/common.cpp` is dependent on things like ArgsManager (which we wish to remove from libbitcoinkernel in the future) and sanity checks. These aren't necessary for libbitcoinkernel so we only extract the portion that is necessary (namely `init::{Set,Unset}Globals()`.
ACKs for top commit:
theuni:
ACK d87784ac87364fc977bbf9769c8bdb72dea8cbf9
vasild:
ACK d87784ac87364fc977bbf9769c8bdb72dea8cbf9
Tree-SHA512: cd6b4923ea1865001b5f0caed9a4ff99c198d22bf74154d935dc09a47fda22ebe585ec912398cea69f722454ed1dbb4898faab5a2d02fb4c5e719c5c8d71a3f9
From 0.14 (2017 Mar) until before 0.19 (2019 Nov), the height of the last
block pruned was returned, subject to a bug if there were blocks left unpruned
due to sharing files with later blocks.
In #15991, this was "fixed" to the current implementation, introducing a new
bug: now, it returns the first *unpruned* block.
Since the user provides the parameter as a block to include in pruning, it
makes more sense to fix the behaviour to match the documentation.
3a171f742c31addf5a343e8a6905054a1fbb12aa logging: fix logging empty threadname (klementtan)
Pull request description:
Currently, `leveldb` background thread does not have a thread name and as a result, an empty thread name is logged.
This PR fixes this by logging thread name as `"unknown"` if the thread name is empty
On master:
```txt
2022-06-02T14:30:38Z [] [leveldb:debug] Generated table #281@0: 1862 keys, 138303 bytes
```
On this PR:
```txt
2022-06-02T14:30:38Z [unknown] [leveldb:debug] Generated table #281@0: 1862 keys, 138303 bytes
```
ACKs for top commit:
laanwj:
Code review ACK 3a171f742c31addf5a343e8a6905054a1fbb12aa
hebasto:
ACK 3a171f742c31addf5a343e8a6905054a1fbb12aa
Tree-SHA512: 0af0fa5c4ddd3640c6dab9595fe9d97f74d0e0f4b41287a6630cf8ac5a21240250e0659ec4ac5a561e888d522f5304bf627104de2aba0fd0a86c1222de0897c2
48262a00f58489d705314ee3c31136133040bb0e Add functional test for block sync from inbound peers (Suhas Daftuar)
0569b5c4bbf8f725e3969d76f7cb081cdf1e4195 Sync chain more readily from inbound peers during IBD (Suhas Daftuar)
Pull request description:
When in IBD, if the honest chain is only known by inbound peers, then we must
eventually sync from them in order to learn it. This change allows us to
perform initial headers sync and fetch blocks from inbound peers, if we have no
blocks in flight.
The restriction on having no blocks in flight means that we will naturally
throttle our block downloads to any such inbound peers that we may be
downloading from, until we leave IBD. This is a tradeoff between preferring
outbound peers for most of our block download, versus making sure we always
eventually will get blocks we need that are only known by inbound peers even
during IBD, as otherwise we may be stuck in IBD indefinitely (which could have
cascading failure on the network, if a large fraction of the network managed to
get stuck in IBD).
Note that the test in the second commit fails on master, without the first commit.
ACKs for top commit:
ajtowns:
ACK 48262a00f58489d705314ee3c31136133040bb0e
sipa:
ACK 48262a00f58489d705314ee3c31136133040bb0e
Tree-SHA512: ffad3a05fa9a32a92226843c9128f52c275e8d51930fde7368badc340227f2ed680561c4c9f2937b4e3bd722474464849ec9b624f912f5e380ce98d71b55764d
8cfb5627d51ecaa1d1e92ec21e2ac56a380c77e6 qt, refactor: add translator comments in `TransactionDesc::FormatTxStatus()` (w0xlt)
Pull request description:
This PR adds translator comments to `TransactionDesc::FormatTxStatus` as suggested in https://github.com/bitcoin-core/gui/pull/552#discussion_r812602741 and https://github.com/bitcoin-core/gui/pull/552#issuecomment-1097294710.
ACKs for top commit:
hebasto:
ACK 8cfb5627d51ecaa1d1e92ec21e2ac56a380c77e6
Tree-SHA512: 2c44b915e6309508f34fc22bb90e3d88ad32ed82fdb3a395f7c6716941edc1b311991140d28e838ad622a7484ed86aedd25e55674857fec8716d9575aed25fa0
