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
This changes all context dependent checks in the parser to be
disjunctions of equality checks, rather than also including inequalities.
This makes sure that adding a new context enum in the future won't change
semantics for existing checks.
The error messages are also made a bit more consistent.
