fa8192f42e rpc: Fail to return undocumented return values (MarcoFalke)
Pull request description:
Currently a few return values are undocumented. This is causing confusion at the least. See for example #18476
Fix this by treating it as an internal bug to return undocumented return values.
ACKs for top commit:
ryanofsky:
Code review ACK fa8192f42e. Only changes: rebase, no const_cast suggestion, and tostring cleanups needed after suggestion
Tree-SHA512: c006905639bafe3045de152b00c34d9864731becb3c4f468bdd61a392f10d7e7cd89a54862c8daa8c11ac4eea0eb5f13b0f647d21e21a0a797b54191cff7238c
This bug doesn't have any effects currently because it only affects
external signer RPCs which aren't currently using the wallet context,
but it does cause an appveyor failure in a upcoming PR:
https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/38512882
This bug is subtle and could have been avoided if JSONRPCRequest didn't
have constructors that were so loose with type checking. Suggested
change
https://github.com/bitcoin/bitcoin/pull/21366#issuecomment-792044351
eliminates these and would be a good followup for a future PR.
This is a non-functional change that replaces the `CNode` on-stack
variables with `CNode` pointers.
The reason for this is that it would allow us to add those `CNode`s
to `CConnman::vNodes[]` which in turn would allow us to check that they
are disconnected properly - a `CNode` object must be in
`CConnman::vNodes[]` in order for its `fDisconnect` flag to be set.
If we store pointers to the on-stack variables in `CConnman` then it
would crash at the end, trying to `delete` them.
2e5f7def22 wallet, rpc: update listdescriptors response format (Ivan Metlushko)
Pull request description:
Update `listdescriptors` response format according to [RPC interface guidelines](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#rpc-interface-guidelines).
This is a follow up for #20226
**Before:**
```
Result:
[ (json array) Response is an array of descriptor objects
{ (json object)
"desc" : "str", (string) Descriptor string representation
"timestamp" : n, (numeric) The creation time of the descriptor
"active" : true|false, (boolean) Activeness flag
"internal" : true|false, (boolean, optional) Whether this is internal or external descriptor; defined only for active descriptors
"range" : [ (json array, optional) Defined only for ranged descriptors
n, (numeric) Range start inclusive
n (numeric) Range end inclusive
],
"next" : n (numeric, optional) The next index to generate addresses from; defined only for ranged descriptors
},
...
]
```
**After:**
```
Result:
{ (json object)
"wallet_name" : "str", (string) Name of wallet this operation was performed on
"descriptors" : [ (json array) Array of descriptor objects
{ (json object)
"desc" : "str", (string) Descriptor string representation
"timestamp" : n, (numeric) The creation time of the descriptor
"active" : true|false, (boolean) Activeness flag
"internal" : true|false, (boolean, optional) Whether this is internal or external descriptor; defined only for active descriptors
"range" : [ (json array, optional) Defined only for ranged descriptors
n, (numeric) Range start inclusive
n (numeric) Range end inclusive
],
"next" : n (numeric, optional) The next index to generate addresses from; defined only for ranged descriptors
},
...
]
}
```
ACKs for top commit:
achow101:
re-ACK 2e5f7def22
meshcollider:
utACK 2e5f7def22
jonatack:
re-ACK 2e5f7def22
Tree-SHA512: 49bf73e46e2a61003ce594a4bfc506eb9592ccb799c2909c43a1a527490a4b4009f78dc09f3d47b4e945d3d7bb3cd2632cf48c5ace5feed5066158cc010dddc1
4eca20d6f7 [doc] correct comment about ATMPW (glozow)
8fa74aeb5b [doc] correct comment in chainparams (glozow)
2f8272c2a4 [doc] GetBestBlock() doesn't do nothing (gzhao408)
Pull request description:
Came across a few misleading comments, wanted to fix them
ACKs for top commit:
jnewbery:
ACK 4eca20d6f7
MarcoFalke:
ACK 4eca20d6f7
laanwj:
Code review ACK 4eca20d6f7
Tree-SHA512: 5bef1f1e7703f304128cf0eb8945e139e031580c99062bbbe15bf4db8443c2ba5a8c65844833132e6646c8980c678fc1d2ab0c63e17105585d583570ee350fd0
73e1f7d754 rpc: document optional fields for getchaintxstats result (Sebastian Falbesoner)
Pull request description:
This mini-PR updates the result help of the `getchaintxstats` RPC by showing the following fields as "optional":
- window_tx_count
- window_interval
- txrate
Help output diff between master and PR branch:
```diff
16,18c16,18
< "window_tx_count" : n, (numeric) The number of transactions in the window. Only returned if "window_block_count" is > 0
< "window_interval" : n, (numeric) The elapsed time in the window in seconds. Only returned if "window_block_count" is > 0
< "txrate" : n (numeric) The average rate of transactions per second in the window. Only returned if "window_interval" is > 0
---
> "window_tx_count" : n, (numeric, optional) The number of transactions in the window. Only returned if "window_block_count" is > 0
> "window_interval" : n, (numeric, optional) The elapsed time in the window in seconds. Only returned if "window_block_count" is > 0
> "txrate" : n (numeric, optional) The average rate of transactions per second in the window. Only returned if "window_interval" is > 0
```
ACKs for top commit:
0xB10C:
ACK 73e1f7d754
Tree-SHA512: 63c8db3e47a3c2d5564d53c564484b95b656e1e5deca1e9841bc90d122d3c81f02fd2b59313fd913ce81b16f7cc2969fe1dd9d6c3e23628b8ac057ea08f55daa
1111896eb7 doc: Merge release notes (MarcoFalke)
faeba9819d rpc: Missing doc updates for bumpfee psbt update (MarcoFalke)
Pull request description:
Stuff missed in #20891. Also merge release notes, so that it doesn't have to be done later.
ACKs for top commit:
fanquake:
ACK 1111896eb7
Tree-SHA512: c9be5a3c944e2981c83546c4761277f1ad5fb9ba97bec80d073db4229924cb48fd23cb5638217c844e05af51d80507718dd201099cbe50819986b3c47c5df7e5
935d488922 [net processing] Refactor MaybeSendAddr() (John Newbery)
01a79ff924 [net processing] Fix overindentation in MaybeSendAddr() (John Newbery)
38c0be5da3 [net processing] Refactor MaybeSendAddr() - early exits (John Newbery)
c87423c58b [net processing] Change MaybeSendAddr() to take a reference (John Newbery)
ad719297f2 [net processing] Extract `addr` send functionality into MaybeSendAddr() (John Newbery)
4ad4abcf07 [net] Change addr send times fields to be guarded by new mutex (John Newbery)
c02fa47baa [net processing] Only call GetTime() once in SendMessages() (John Newbery)
Pull request description:
This continues the work of moving application layer data into net_processing. It refactors `addr` send functionality into its own function `MaybeSendAddr()` and flattens/simplifies the code. Isolating and simplifying the addr handling code makes subsequent changes (which will move addr data and logic into net processing) easier to review.
This is a pure refactor. There are no functional changes.
For motivation of the project, see #19398.
ACKs for top commit:
sipa:
utACK 935d488922
hebasto:
ACK 935d488922, I have reviewed the code and it looks OK, I agree it can be merged.
MarcoFalke:
review ACK 935d488922🐑
Tree-SHA512: 4e9dc84603147e74f479a211b42bcf315bdf5d14c21c08cf0b17d6c252775b90b012f0e0d834f1a607ed63c7ed5c63d5cf49b134344e7b64a1695bfcff111c92
Throwing an exception from a slot invoked by Qt's signal-slot connection
mechanism is considered undefined behavior, unless it is handled within
the slot. The GUIUtil::ExceptionSafeConnect function should be used for
exception handling within slots.
916ab0195d remove unused class util::Ref and its unit test (Sebastian Falbesoner)
8dbb87a393 refactor: replace util::Ref by std::any (C++17) (Sebastian Falbesoner)
95cccf8a4b util: introduce helper AnyPtr to access std::any instances (Sebastian Falbesoner)
Pull request description:
As described in `util/ref.h`: "_This implements a small subset of the functionality in C++17's std::any class, and **can be dropped when the project updates to C++17**_". For accessing the contained object of a `std::any` instance, a helper template function `AnyPtr` is introduced (thanks to ryanofsky).
ACKs for top commit:
hebasto:
re-ACK 916ab0195d, with command
ryanofsky:
Code review ACK 916ab0195d. Changes since last review: rebase and replacing types with `auto`. I might have used `const auto*` and `auto*` instead of plain `auto` because I think the qualifiers are useful, but this is all good.
Tree-SHA512: fe2c3e4f5726f8ad40c61128339bb24ad11d2c261f71f7b934b1efe3e3279df14046452b0d9b566917ef61d5c7e0fd96ccbf35ff810357e305710f5002c27d47
Changes to make MaybeSendAddr simpler and easier to maintain/update:
- assert invariant that node.vAddrToSend.size() can never exceed
MAX_ADDR_TO_SEND
- erase known addresses from vAddrToSend in one pass
- no check for (vAddr.size() >= MAX_ADDR_TO_SEND) during iteration,
since vAddr can never exceed MAX_ADDR_TO_SEND.
4783115fd4 net: add ifaddrs.h include (fanquake)
879215e665 build: check if -lsocket is required with *ifaddrs (fanquake)
87deac66aa rand: only try and use freeifaddrs if available (fanquake)
Pull request description:
Fixes#21485 by linking against `-lsocket` when it's required for using `*ifaddrs` functions.
ACKs for top commit:
laanwj:
Code review ACK 4783115fd4
hebasto:
ACK 4783115fd4, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 4542e036e9b029de970eff8a9230fe45d9204bb22313d075f474295d49bdaf1f1cbb36c0c6e2fa8dbbcdba518d8d3a68a6116ce304b82414315f333baf9af0e4
fa4926cca6 fuzz: [refactor] Use IsValidFlagCombination in signature_checker fuzz target (MarcoFalke)
eeee8f5be1 fuzz: Removed unused try-catch in coins_view (MarcoFalke)
fa98f3f66e fuzz: [refactor] Use ConsumeScript in signature_checker fuzz target (MarcoFalke)
Pull request description:
Some small refactors to remove unused and redundant fuzz code
ACKs for top commit:
practicalswift:
cr re-ACK fa4926cca6
Tree-SHA512: eb07a2140caad7b31495b76385fc7634cf5b6daa4947f430ebb127eb1375583dc11e541a0a42d0e5d93d430480b8a815b93974450fd5ed897528a2d47c752f86
40316a37cb test: add I2P test for a runaway SAM proxy (Vasil Dimov)
2d8ac77970 fuzz: add tests for the I2P Session public interface (Vasil Dimov)
9947e44de0 i2p: use pointers to Sock to accommodate mocking (Vasil Dimov)
82d360b5a8 net: change ConnectSocketDirectly() to take a Sock argument (Vasil Dimov)
b5861100f8 net: add connect() and getsockopt() wrappers to Sock (Vasil Dimov)
5a887d49b2 fuzz: avoid FuzzedSock::Recv() repeated errors with EAGAIN (Vasil Dimov)
3088f83d01 fuzz: extend FuzzedSock::Recv() to support MSG_PEEK (Vasil Dimov)
9b05c49ade fuzz: implement unimplemented FuzzedSock methods (Vasil Dimov)
Pull request description:
Change the networking code and the I2P code to be fully mockable and use `FuzzedSocket` to fuzz the I2P methods `Listen()`, `Accept()` and `Connect()`.
Add a mocked `Sock` implementation that returns a predefined data on reads and use it for a regression unit test for the bug fixed in https://github.com/bitcoin/bitcoin/pull/21407.
ACKs for top commit:
practicalswift:
Tested ACK 40316a37cb
MarcoFalke:
Concept ACK 40316a37cb
jonatack:
re-ACK 40316a37cb reviewed `git range-diff 01bb3afb 23c861d 40316a3` and the new unit test commit, debug built, ran unit tests, ran bitcoind with an I2P service and network operation with seven I2P peers (2 in, 5 out) is looking nominal
laanwj:
Code review ACK 40316a37cb
Tree-SHA512: 7fc4f129849e16e0c7e16662d9f4d35dfcc369bb31450ee369a2b97bdca95285533bee7787983e881e5a3d248f912afb42b4a2299d5860ace7129b0b19623cc8
0cca08a8ee Add unit test coverage for our onion peer eviction protection (Jon Atack)
caa21f586f Protect onion+localhost peers in ProtectEvictionCandidatesByRatio() (Jon Atack)
8f1a53eb02 Use EraseLastKElements() throughout SelectNodeToEvict() (Jon Atack)
8b1e156143 Add m_inbound_onion to AttemptToEvictConnection() (Jon Atack)
72e30e8e03 Add unit tests for ProtectEvictionCandidatesByRatio() (Jon Atack)
ca63b53ecd Use std::unordered_set instead of std::vector in IsEvicted() (Jon Atack)
41f84d5ecc Move peer eviction tests to a separate test file (Jon Atack)
f126cbd6de Extract ProtectEvictionCandidatesByRatio from SelectNodeToEvict (Jon Atack)
Pull request description:
Now that #19991 and #20210 have been merged, we can determine inbound onion peers using `CNode::m_inbound_onion` and add it to the localhost peers protection in `AttemptToEvictConnection`, which was added in #19670 to address issue #19500.
Update 28 February 2021: I've updated this to follow gmaxwell's suggestion in https://github.com/bitcoin/bitcoin/pull/20197#issuecomment-713865992.
This branch now protects up to 1/4 onion peers (connected via our tor control service), if any, sorted by longest uptime. If any (or all) onion slots remain after that operation, they are then allocated to protect localhost peers, or a minimum of 2 localhost peers in the case that no onion slots remain and 2 or more onion peers were protected, sorted as before by longest uptime.
This patch also adds test coverage for the longest uptime, localhost, and onion peer eviction protection logic to build on the welcome initial unit testing of #20477.
Suggest reviewing the commits that move code with `colorMoved = dimmed-zebra` and `colorMovedWs = allow-indentation-change`.
Closes#11537.
ACKs for top commit:
laanwj:
Code review ACK 0cca08a8ee
vasild:
ACK 0cca08a8ee
Tree-SHA512: 2f5a63f942acaae7882920fc61f0185dcd51da85e5b736df9d1fc72343726dd17da740e02f30fa5dc5eb3b2d8345707aed96031bec143d48a2497a610aa19abd
3fc06d3d7b [net] remove fUpdateConnectionTime from FinalizeNode (John Newbery)
7c4cc67c0c [net] remove CConnman::AddNewAddresses (John Newbery)
bcd7f30b79 [net] remove CConnman::MarkAddressGood (John Newbery)
8073673dbc [net] remove CConnman::SetServices (John Newbery)
392a95d393 [net_processing] Keep addrman reference in PeerManager (John Newbery)
1c25adf6d2 [net] Construct addrman outside connman (John Newbery)
Pull request description:
Addrman is currently a member variable of connman. Make it a top-level component with lifetime owned by node.context, and add a reference to addrman in peerman. This allows us to eliminate some functions in connman that are simply forwarding requests to addrman, and simplifies the connman-peerman interface.
By constructing the addrman in init, we can also add parameters to the ctor, which allows us to test it better. See #20233, where we enable consistency checking for addrman in our functional tests.
ACKs for top commit:
MarcoFalke:
re-ACK 3fc06d3d7b only change is squash 🏀
vasild:
ACK 3fc06d3d7b
Tree-SHA512: 17662c65cbedcd9bd1c194914bc4bb4216f4e3581a06222de78f026d6796f1da6fe3e0bf28c2d26a102a12ad4fbf13f815944a297f000e3acf46faea42855e07
90ae3d8ca6 doc: Add release notes for -deprecatedrpc=addresses and bitcoin-tx (Michael Dietz)
085b3a7299 rpc: deprecate `addresses` and `reqSigs` from rpc outputs (Michael Dietz)
Pull request description:
Considering the limited applicability of `reqSigs` and the confusing output of `1` in all cases except bare multisig, the `addresses` and `reqSigs` outputs are removed for all rpc commands.
1) add a new sane "address" field (for outputs that have an identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact (with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely always.
Note: Some light refactoring done to allow us to very easily delete a few chunks of code (marked with TODOs) when we remove this deprecated behavior.
Using `IsDeprecatedRPCEnabled` in core_write.cpp caused some circular dependencies involving core_io
Circular dependencies were caused by rpc/util unnecessarily importing node/coinstats and node/transaction. Really what rpc/util needs are some fundamental type/helper-function definitions. So this was cleaned up to make more sense.
This fixes#20102.
ACKs for top commit:
MarcoFalke:
re-ACK 90ae3d8ca6📢
Tree-SHA512: 8ffb617053b5f4a8b055da17c06711fd19632e0037d71c4c8135e50c8cd7a19163989484e4e0f17a6cc48bd597f04ecbfd609aef54b7d1d1e76a784214fcf72a
Add early exit guard clauses if node.RelayAddrsWithConn() is false or if
current_time < node.m_next_addr_send. Add comments.
This commit leaves some lines over-indented. Those will be fixed in a
subsequent whitespace-only commit.
beead33a21 [test] no send feefilters when txrelay is turned off (glozow)
18a9b27dd6 p2p: Don't send FEEFILTER in blocksonly mode (Martin Zumsande)
Pull request description:
The purpose of FEEFILTER messages (BIP 133) is to inform our peers that we do not want transactions below a specified fee rate.
In blocksonly mode, we do not want our peer to send us any transactions at all (and will disconnect if a peer still sends a transaction INV or TX).
Therefore, I don't think that it makes sense to send FEEFILTER messages every 10 minutes on average in blocksonly mode - this PR disables it.
Note that on block-relay-only connections, FEEFILTER is already disabled, just not in blocksonly mode.
ACKs for top commit:
glozow:
re ACK beead33a21🙂 thanks for adding the test!
amitiuttarwar:
reACK beead33a21
MarcoFalke:
review ACK beead33a21
jnewbery:
reACK beead33a21
Tree-SHA512: e748cd52fe23d647fa49008b020389956ac508e16ce9fd108d8afb773bff95788298ae229162bd70215d7246fc25c796484966dc05890b0b4ef601f9cd35628b
9ac86bcc0d test: remove qt byteswap compattests (fanquake)
Pull request description:
These were added as part of #9366 when with fixing issues with Protobuf.
Now that we no-longer use Protobuf, there's no reason to maintain a duplicate set of byteswap tests in the qt tests. Our other set of byteswap tests are here: https://github.com/bitcoin/bitcoin/blob/master/src/test/bswap_tests.cpp.
ACKs for top commit:
laanwj:
Code review ACK 9ac86bcc0d
Tree-SHA512: 72ba131a5f8fbd9fdbbc4e1f95baa794496c960b12e0271700c632c6511b7e1b331e8db07a201838b4d56b2aeeb43d4de4e10265ea07ab14241307fa14d3342e
dc4551c22c remove incompatibility release note for darkmode on macos (Sylvain Goumy)
303cfc6227 allow darkmode on macos build (Sylvain Goumy)
78f75a2d60 Allow icon colorization on mac os to better support dark mode (Uplab)
Pull request description:
Allow icons to be colorized on macOS to support native Dark mode color scheme.
Rendering on macOS Big Sur before PR:
Rendering on macOS Big Sur after PR:
Light mode stay visually unchanged.
<del>Note, that this currently only affect the build from source, as the macos dmg includes an attributes to force light color scheme on macos windows (see https://github.com/bitcoin/bitcoin/pull/14593). </del>
<del>But once all glitches are fixed, we will be able to remove this temporary fix. </del>
Edit: this PR is know including the removal of `NSRequiresAquaSystemAppearance` on Info.plist file so that the color fix is apply to every build.
Linked issues: #68#136
ACKs for top commit:
hebasto:
re-ACK dc4551c22c
jarolrod:
ACK dc4551c22c
Tree-SHA512: 1c3a4dec796063e61fcaf80112afc2b15c8669a1cd30ebd537cea96647c20215f8f80289719f905820bb0c490c8c1f94bfae4bb32f9c6d1fdd4e8f199ebb559f
732c7bddeb tests: Add test for CNetAddr::ToString IPv6 address formatting (RFC 5952) (practicalswift)
Pull request description:
Test that `CNetAddr::ToString` formats IPv6 addresses with zero compression and canonicalisation as described in [RFC 5952 ("A Recommendation for IPv6 Address Text Representation")](https://tools.ietf.org/html/rfc5952).
Solving #21466 will hopefully be trivial with the ability to check zero compression correctness against these tests.
ACKs for top commit:
vasild:
ACK 732c7bddeb
Tree-SHA512: 31a1378aa435ba4171490a2e15d7280a175292270eb001b47d367e010c6ac9b83420b82bbeab22211f8f500c69e21878047c87adf216263b3420b6bb2a5d2bfb
These were added as part of #9366 to fix issues with Protobuf.
Now that we no-longer use Protobuf, there's no reason to maintain a
duplicate set of byteswap tests for qt.
257f55c119 qt, refactor: Drop redundant setEditTriggers(NoEditTriggers) calls (Hennadii Stepanov)
Pull request description:
The models of the both views have no `Qt::ItemIsEditable` flag:
3c87dbe95c/src/qt/peertablemodel.cpp (L218-L224)3c87dbe95c/src/qt/bantablemodel.cpp (L148-L154)
ACKs for top commit:
Talkless:
utACK 257f55c119, seems reasonable.
jarolrod:
ACK 257f55c119, looks correct.
Tree-SHA512: 4356e4d785055935fba452488a5d97ed95995def97b26ab18af43a545835f9e9d4c347e4cad7952aa725179cf6e775a2208c48730feebf40e3b1a7ba5f402af0
915e34112b qt: fix issue when disabling the auto-enabled blank wallet checkbox (Jarol Rodriguez)
Pull request description:
As detailed by #151, On `master` a user can create the confusing scenario where you have a disabled `Encrypt Wallet` checkbox and a selected `Disable Private Keys` checkbox after unselecting the auto-enabled `Blank Wallet` checkbox.
This commit makes it so that when the `Blank Wallet` checkbox is auto-selected after the user selects `Disable Private keys`, unselecting it will also unselect the `Disable Private Keys` checkbox, which in turn re-enables the `Encrypt Wallet` checkbox.
Below are screenshots comparing the behavior of selecting `Disable Private Keys` then unselecting the `Blank Wallet` between `master` and this `PR`:
**Master:**
| Select `Disable Private Keys` | Unselect `Blank Wallet` |
| ----------------------------- | ------------------------ |
|  |  |
**PR:**
| Select `Disable Private Keys` | Unselect `Blank Wallet` |
| ----------------------------- | ------------------------ |
|  |  |
ACKs for top commit:
hebasto:
ACK 915e34112b
Talkless:
ACK 915e34112b
Tree-SHA512: ce6ecbc35b94a08cabf0b8a24dbdfc874d82cc8918cc8623dce8172c7fc9c75d63a13b036bae5f7ab2c090f8d020574a542285d1651600813faf5d91e2506a8d
4f2653a890 test: Use deterministic chain in utxo set hash test (Fabian Jahr)
4973c5175c test: Remove wallet dependency of utxo set hash test (Fabian Jahr)
1a27af1d7b rpc: Improve gettxoutsetinfo help (Fabian Jahr)
Pull request description:
Follow-ups to #19145:
- Small improvement on the help text of RPC gettxoutsetinfo
- Using deterministic blockchain in the test `functional/feature_utxo_set_hash.py`
- Removing wallet dependency in the test `functional/feature_utxo_set_hash.py`
Split out of #19521.
ACKs for top commit:
MarcoFalke:
review ACK 4f2653a890👲
Tree-SHA512: 92927b3aa22b6324eb4fc9d346755313dec44d973aa69a0ebf80a8569b5f3a7cf3539721ebdba183737534b9e29b3e33f412515890f0d0b819878032a3bba8f9
fa818ca202 fuzz: [refactor] Use PickValue where possible (MarcoFalke)
Pull request description:
`PickValue` is a bit less typing, so I think it should be used where possible
ACKs for top commit:
practicalswift:
cr ACK fa818ca202: patch looks correct and `PickValue` is better :)
Tree-SHA512: 49ed030694e3b7676654f1615f033287d26e2f0bc29647e1db56e0d84e14d29080f3e1898f5df8d644d834b8ded3ce713d2425ea86a37c9279d01f86ad03c202
246774e264 depends: fix Qt precompiled headers bug (Igor Cota)
8e7ad4146d depends: disable Qt Vulkan support on Android (Igor Cota)
ba46adaa1a CI: add Android APK build to cirrus (Igor Cota)
7563720e30 CI: add Android APK build script (Igor Cota)
ebfb10cb75 Qt: add Android packaging support (Igor Cota)
Pull request description:
This PR is the third and final piece of the basic Android support puzzle - it depends on https://github.com/bitcoin/bitcoin/pull/16110 and is related to https://github.com/bitcoin/bitcoin/pull/16883. It introduces an `android` directory under `qt` and a simple way to build an Android package of `bitcoin-qt`:
1. Build depends for Android as described in the [README](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md)
2. Configure with one of the resulting prefixes
3. Run `make && make apk` in `src/qt`
The resulting APK files will be in `android/build/outputs/apk`. You can install them manually or with [adb](https://developer.android.com/studio/command-line/adb). One can also open the `android` directory in Android Studio for that integrated development and debugging experience. `BitcoinQtActivity` is your starting point.
Under the hood makefile `apk` target:
1. Renames the `bitcoin-qt` binary to `libbitcoin-qt.so` and copies it over to a folder under `android/libs` depending on which prefix and corresponding [ABI](https://developer.android.com/ndk/guides/abis.html#sa) `bitcoin-qt` was built for
2. Takes `libc++_shared.so` from the Android NDK and puts in the same place. It [must be included](https://developer.android.com/ndk/guides/cpp-support) in the APK
3. Extracts Qt for Android Java support files from the `qtbase` archive in `depends/sources` to `android/src`
There is also just a tiny bit of `ifdef`'d code to make the Qt Widgets menus usable. It's not pretty but it works and is a stepping stone towards https://github.com/bitcoin/bitcoin/pull/16883.
ACKs for top commit:
MarcoFalke:
cr ACK 246774e264
laanwj:
Code review ACK 246774e264
Tree-SHA512: ba30a746576a167545223c35a51ae60bb0838818779fc152c210f5af1413961b2a6ab6af520ff92cbc8dcd5dcb663e81ca960f021218430c1f76397ed4cead6c
804ac10631 remove unnecessary newline from initWarning() argument (Larry Ruane)
Pull request description:
Run: `src/bitcoind -wallet=nosuchfile`
Without this patch, `debug.log` contains:
```
2021-03-23T21:19:16Z init message: Verifying wallet(s)...
2021-03-23T21:19:16Z Warning: Skipping -wallet path that doesn't exist. Failed to load database path '/home/larry/.bitcoin/wallets/nosuchfile'. Path does not exist.
2021-03-23T21:19:16Z init message: Loading banlist...
```
With this patch, the empty line isn't present. This PR fixes a similar problem with `src/bitcoind -conf=nosuchfile`
ACKs for top commit:
practicalswift:
cr ACK 804ac10631: patch looks correct!
jarolrod:
tACK 804ac10631, nice catch!
theStack:
Code-review ACK 804ac10631
Tree-SHA512: dfcbaaa72ca24ac40233ac56840cfba8827853711d3df6e229ce940686f2ebf8bf0560bafcaa73a4d82d179a5050af0d3cabdc47b3b1dfd6aaadf718a6635f11
This commit makes it so that when the `Blank Wallet` checkbox is auto-selected after the user selects 'Disable Private' keys, unselecting it will also unselect the 'Disable Private Keys' checkbox, which in turn re-enables the 'Encrypt Wallet' checkbox.
fac921f23f fuzz: Fix tx_pool target to properly fuzz immature outpoints (MarcoFalke)
fa2b95f861 fuzz: Style fixups (MarcoFalke)
Pull request description:
Also includes a commit for minor style fixups
ACKs for top commit:
glozow:
utACK fac921f23f this fixes it 👍
Tree-SHA512: 1575ba115b2009b653921511c163bd846cd381d6fc92b04a899c0686d23a02bdcdd95c81776b515b80ae187bcec3ccaca3aa88fcecbec888f73ca2d875eef506
d09ebc4723 Fix wrong(1024) divisor for 1000-based prefixes (wodry)
Pull request description:
v.0.21.0
I saw in the GUI peer window in the "received" column `1007 KB`, and after increasing to >=1024 I guess, it switched to `1 MB`. I would have expected the display unit to change from KB to MB already at value >=1000.
I looked into the code, and the values appear to be power-of-2 byte values, so the switching at >=1024 and not >=1000 seems correct.
But the unit display is not precisely correct, binary prefixes should be used for power-of-2 byte values.
To be correct, this PR changes ~~KB/MB/GB to KiB/MiB/GiB.~~ KB to kB and the divisor from 1024 to 1000.
ACKs for top commit:
hebasto:
ACK d09ebc4723, tested on Linux Mint 20.1 (Qt 5.12.8) the both "Network Traffic" and "Peers" tabs of the "Node Window".
jarolrod:
ACK d09ebc4723
leonardojobim:
Tested ACK d09ebc4723 on Ubuntu 20.04 Qt 5.12.8
Tree-SHA512: 8f830b08cc3fd36dc8a18f1192959fe55d1644938044bf31d770f7c3bf8475fba6da5019a2d2024d5b2c81a8dab112f360c555367814a14f4d05c89d130f25b0
1) add a new sane "address" field (for outputs that have an
identifiable address, which doesn't include bare multisig)
2) with -deprecatedrpc: leave "reqSigs" and "addresses" intact
(with all weird/wrong behavior they have now)
3) without -deprecatedrpc: drop "reqSigs" and "addresses" entirely,
always.
1404c57403 [doc] Coin: explain that IsSpent() can also mean never existed (Sjors Provoost)
Pull request description:
This can be especially confusing where `AccessCoin()` is used with logic like this:
```c++
while (iter.n < MAX_OUTPUTS_PER_BLOCK) {
const Coin& alternate = view.AccessCoin(iter);
if (!alternate.IsSpent()) return alternate;
```
ACKs for top commit:
practicalswift:
ACK 1404c57403
MarcoFalke:
ACK 1404c57403
jnewbery:
utACK 1404c57403
Tree-SHA512: 418618dd7e08bd5cc8360e3501d0f57e34100e5101ad3b8e0a819923fa860f44c7f2fada0f8447a1af3c2601fd72bfe619b91ff2f26f7133ceaeb0c98b017b12
55554463c1 fuzz: Use ConsumeWeakEnum in addrman for service flags (MarcoFalke)
Pull request description:
This has minimally better performance. Reported by me in https://github.com/bitcoin/bitcoin/pull/20228#discussion_r598081787
ACKs for top commit:
practicalswift:
Tested ACK 55554463c1
vasild:
ACK 55554463c1
Tree-SHA512: 4e5f51fe4f2bd7b2f37d0690e41203341ba45c0c9bc9247449cd26cfb5f77dc2ec61df3e4963276f68694e4b3ca3d0a76367a51c4d775501edeb3224d305a261
fa4cebadcf util: Make Assume() usable as unary expression (MarcoFalke)
Pull request description:
Assume shouldn't behave different at the call site depending on build flags. Currently compilation fails if it is used as expression. Fix that by using the lambda approach from `Assert()` without the `assert()`.
ACKs for top commit:
jnewbery:
ACK fa4cebadcf
practicalswift:
cr ACK fa4cebadcf: patch looks correct and commit hash starts with `fa`
Tree-SHA512: 9ec9ac8d410cdaf5e4e28df571a89e3d23d38e05a7027bb726cae3da6e9314734277e5a218e9e090cc17e10db763da71052c229ad642077ca5824ee42022f3ed
5294f0d5a9 refactor: return std::nullopt instead of {} (fanquake)
Pull request description:
In #21415 [we decided](https://github.com/bitcoin/bitcoin/pull/21415#issuecomment-800236640) to return `std::optional` rather than `{}` for
uninitialized values. This PR replaces the two remaining usages of `{}`
with `std::nullopt`.
As a side-effect, this also quells the spurious GCC 10.2.x warning that
we've had reported quite a few times. i.e #21318, #21248, #20797.
```bash
txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’:
txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
898 | return {};
| ^
```
ACKs for top commit:
hebasto:
ACK 5294f0d5a9, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 5b776be79ab26e5a3a5fc2b463b394ea5ce6797ed5558424873fa4ecee2898170eff76d6da9d69394d28f8f98974117fc63b922a3e19c52f5294c83073e79bb0
In #21415 we decided to return `std::optional` rather than `{}` for
uninitialized values. This PR repalces the two remaining usages of `{}`
with `std::nullopt`.
As a side-effect, this also quells the spurious GCC 10.2.x warning that
we've had reported quite a few times. i.e #21318, #21248, #20797.
```bash
txmempool.cpp: In member function ‘CTxMemPool::setEntries CTxMemPool::GetIterSet(const std::set<uint256>&) const’:
txmempool.cpp:898:13: warning: ‘<anonymous>’ may be used uninitialized in this function [-Wmaybe-uninitialized]
898 | return {};
| ^
```
Introduce an android directory under qt and allow one to package bitcoin-qt for Android by running make apk.
Add bitcoin-qt Android build instructions.
3d086f42ab test: add ParseUInt16() test coverage (Jon Atack)
Pull request description:
`ParseUInt16()` was just added in #21328.
ACKs for top commit:
practicalswift:
cr ACK 3d086f42ab: patch looks correct & more coverage is better than less coverage
Tree-SHA512: bf7f96deb7c1531419565907f0ea8a8e32b368d4b823a3e80928b2c118edbf643ea06e357b4b5504a89f855caeed289daa9f823c740231ed6ad1b8ed00285ce8
52a43b0c7d build: Fix fuzz-cuckoocache cross-compiling for Windows with DEBUG=1 (Hennadii Stepanov)
Pull request description:
Fix#21348.
ACKs for top commit:
practicalswift:
Tested ACK 52a43b0c7d
Tree-SHA512: 6592f829edfb740a1e9d0691acf04b2372e91b0a53ca395b08350cb0b80031d3b55fa7331bdaddf857d450eb30b605af9fe8fe02559cda19374a48f9634fae70
787df19b09 validation: don't try to invalidate genesis block (Sebastian Falbesoner)
Pull request description:
In the block invalidation method (`CChainState::InvalidateBlock`), the code for creating the candidate block map assumes that the passed block's previous block (`pindex->pprev`) is available and otherwise segfaults due to null-pointer deference in `CBlockIndexWorkComparator()` (see analysis by practicalswift in #20914), i.e. it doesn't work with the genesis block. Rather than analyzing all possible code paths and implications for this corner case, simply fail early if the genesis block is passed.
Fixes#20914.
ACKs for top commit:
sipa:
ACK 787df19b09. Tested invalidation of generic on regtest.
practicalswift:
Tested ACK 787df19b09
Tree-SHA512: 978be7cf2bd1c1faebfe945d191ac77dea72791bea826459abd308f77c74c5991efee495a38817c306e488ecd5208b5c888df7d9d044132dd9a06bbbdb256b6c
8dd5946c0b add functional test (Larry Ruane)
b5a80fa7e4 util: Handle HTTP_SERVICE_UNAVAILABLE in bitcoin-cli (Hennadii Stepanov)
Pull request description:
If `bitcoind` is processing 16 RPC requests, attempting to submit another request using `bitcoin-cli` produces this less-than-helpful error message: `error: couldn't parse reply from server`. This PR changes the error to: `error: server response: Work queue depth exceeded`.
ACKs for top commit:
fjahr:
tACK 8dd5946c0b
luke-jr:
utACK 8dd5946c0b (no changes since previous utACK)
hebasto:
re-ACK 8dd5946c0b, only suggested changes since my [previous](https://github.com/bitcoin/bitcoin/pull/18335#pullrequestreview-460621350) review.
darosior:
ACK 8dd5946c0b
Tree-SHA512: 33e25f6ff05d9b56fae2bdb68b132557bb8e995f5438ac4fbbc53c304c5152a98aa43c43600c31d8a6a2830cbd48bf8ec7d89dce50190b29ec00a43830126913
52dd40a9fe test: add missing netaddress include headers (Jon Atack)
6f09c0f6b5 util: add missing braces and apply clang format to SplitHostPort() (Jon Atack)
2875a764f7 util: add ParseUInt16(), use it in SplitHostPort() (Jon Atack)
6423c8175f p2p, refactor: pass and use uint16_t CService::port as uint16_t (Jon Atack)
Pull request description:
As noticed during review today in https://github.com/bitcoin/bitcoin/pull/20685#discussion_r584873708 of the upcoming I2P network support, `CService::port` is `uint16_t` but is passed around the codebase and into the ctors as `int`, which causes uneeded conversions and casts. We can avoid these (including in the incoming I2P code without further changes to it) by using ports with the correct type. The remaining conversions are pushed out to the user input boundaries where they can be range-checked and raise with user feedback in the next patch.
ACKs for top commit:
practicalswift:
cr ACK 52dd40a9fe: patch looks correct
MarcoFalke:
cr ACK 52dd40a9fe
vasild:
ACK 52dd40a9fe
Tree-SHA512: 203c1cab3189a206c55ecada77b9548b810281cdc533252b8e3330ae0606b467731c75f730ce9deb07cbaab66facf97e1ffd2051084ff9077cba6750366b0432
Now that we have a reliable way to detect inbound onion peers, this commit
updates our existing eviction protection of 1/4 localhost peers to instead
protect up to 1/4 onion peers (connected via our tor control service), sorted by
longest uptime. Any remaining slots of the 1/4 are then allocated to protect
localhost peers, or 2 localhost peers if no slots remain and 2 or more onion
peers are protected, sorted by longest uptime.
The goal is to avoid penalizing onion peers, due to their higher min ping times
relative to IPv4 and IPv6 peers, and improve our diversity of peer connections.
Thank you to Gregory Maxwell, Suhas Daftuar, Vasil Dimov and Pieter Wuille
for valuable review feedback that shaped the direction.
and an `m_is_onion` struct member to NodeEvictionCandidate and tests.
We'll use these in the peer eviction logic to protect inbound onion peers
in addition to the existing protection of localhost peers.
An unordered set can tell if an element is present in ~O(1) time (constant on
average, worst case linear to the size of the container), which speeds up and
simplifies the lookup in IsEvicted().
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
out of net_tests, because the eviction tests:
- are a different domain of test coverage, with different dependencies
- run more slowly than the net tests
- will be growing in size, in this PR branch and in the future, as eviction
test coverage is improved
to allow deterministic unit testing of the ratio-based peer eviction protection
logic, which protects peers having longer connection times and those connected
via higher-latency networks.
Add documentation.
fa81773243 style-only: Remove whitespace (MarcoFalke)
fae77b9e6d net: Simplify ProcessGetBlockData execution by removing send flag. (Patrick Strateman)
fae7c0429f log: Clarify that block request below NODE_NETWORK_LIMITED_MIN_BLOCKS disconnects (MarcoFalke)
Pull request description:
* Clarify that "ignoring" really means "disconnect" in the log
* Revive a refactor I took from #13670
ACKs for top commit:
jnewbery:
utACK fa81773243
sipa:
utACK fa81773243
Tree-SHA512: 0a4fcb979cb82c4e26012881eeaf903c38dfbb85d461476c01e35294760744746a79c48ffad827fe31c1b830f40c6e4240529c71e375146e4d0313c3b7d784ca
fa2a80bf12 refactor: Pass PeerManagerImpl members only once (MarcoFalke)
Pull request description:
Member variables are already passed to methods via `this`, so no need to pass them another time as function parameter.
ACKs for top commit:
jnewbery:
utACK fa2a80bf12
amitiuttarwar:
utACK fa2a80bf12
Tree-SHA512: 1743825c7560cc748235e3db03e4cea02ad1f670f1b898d7757da644f12693ba9bb2d3eb09b64b3d10dd2e68f52dea31e26d5e97bdc013759baa0515d3c7055c
680eb56d82 [net processing] Don't pass CConnman to RelayTransactions (John Newbery)
a38a4e8f03 [net processing] Move RelayTransaction into PeerManager (John Newbery)
Pull request description:
This is the first part of #21160. It moves the RelayTransaction() function to be a member function of the PeerManager class. This is required in order to move the transaction inventory data into the Peer object, since Peer objects are only accessible from within PeerManager.
ACKs for top commit:
ajtowns:
ACK 680eb56d82
Tree-SHA512: 8c93491a4392b6369bb7f090de326a63cd62a088de59026e202f226f64ded50a0cf1a95ed703328860f02a9d2f64d3a87ca1bca9a6075b978bd111d384766235
ebc4ab721b refactor: post Optional<> removal cleanups (fanquake)
57e980d13c scripted-diff: remove Optional & nullopt (fanquake)
Pull request description:
Same rationale & motivation as #21404, which turned out to be quite low in the number of potential conflicts. Lets see what the bot has to say here.
ACKs for top commit:
practicalswift:
cr ACK ebc4ab721b: patch looks correct
jnewbery:
utACK ebc4ab721b
laanwj:
Code review ACK ebc4ab721b
Tree-SHA512: 550fbeef09b9d35ddefaa805d1755c18c8fd499c4b0f77ebfece8c20296a7abd1cf6c699e2261f92fe3552deeb7555ec2a2287ffe3ab9e98bb9f8612a4d43be3
57ff5a42ab doc: specify minimum HWI version (Sjors Provoost)
03308b2bfa rpc: don't require wallet for enumeratesigners (Sjors Provoost)
Pull request description:
HWI just released 2.0. See https://github.com/bitcoin-core/HWI/releases/tag/2.0.0
As of #16546 we already rely on features that are in 2.0 and not in the previous 1.* releases:
* `--chain` param
This shouldn't be a problem, because HWI 2.0 has been released before we release v22.
Misc improvements:
* document that HWI 2.0 is required
* drop wallet requirement for `enumeratesigners`
ACKs for top commit:
achow101:
Code Review ACK 57ff5a42ab
Tree-SHA512: 3fb6ba20894e52a116f89525a5f5a1f61d363ccd904e1cffd0e6d095640fc6d2edf0388cd6ae20f83bbc31e5f458255ec090b6e823798d426eba3e45b4336bf9
f9cd2bfbcc Rename CoinSelectionParams::effective_fee to m_effective_feerate (Andrew Chow)
bdd0c2934b wallet: Move discard feerate fetching to CreateTransaction (Andrew Chow)
448d04b931 wallet: Move long term feerate setting to CreateTransaction (Andrew Chow)
e2f429e6bb wallet: Replace nFeeRateNeeded with effective_fee (Andrew Chow)
1a6a0b0dfb wallet: Use existing feerate instead of getting a new one (Andrew Chow)
Pull request description:
During coin selection, there are various places where we need to have a feerate. We need the feerate for the transaction itself, the discard fee rate, and long term feerate. Fetching these each time we need them can lead to a race condition where two feerates that should be the same are actually different. One particular instance where this can happen is during the loop in `CreateTransactionInternal`. After inputs are chosen, the expected transaction fee is calculated using a newly fetched feerate. If `pick_new_inputs == false`, the loop will go again with the assumption that the fee for the transaction remains the same. However because the feerate is fetched again, it is possible that it actually isn't and this causes coin selection to fail.
Instead of fetching the feerate each time it is needed, we fetch them all at once at the top of `CreateTransactionInternal`, store them in `CoinSelectionParams`, and use them where needed.
While some of these fee rates probably don't need this caching, I've done it for consistency and the guarantee that they remain the same.
Fixes#19229
ACKs for top commit:
glozow:
reACK f9cd2bfbcc
fjahr:
Code review re-ACK f9cd2bfbcc
Xekyo:
tACK f9cd2bfbcc
meshcollider:
Code review + test run ACK f9cd2bfbcc
Tree-SHA512: be83ff64ba473c3cdd3469c812e214659b6e2a9584c22ed2b1595618fce0d4b35d0901e61068cd1069fc1a8fb911db01dd7312d05c3b8cbafbe2504ab7a3e863
Instead of fetching the discard feerate for each SelectCoinsMinConf
iteration, fetch and cache it once during CreateTransaction so that it
is shared for each SelectCoinsMinConf through
coin_selection_params.m_discard_feerate.
Does not change behavior.
Instead of setting the long term feerate for each SelectCoinsMinConf
iteration, set it once during CreateTransaction and let it be shared
with each SelectCoinsMinConf through
coin_selection_params.m_long_term_feerate.
Does not change behavior.
