fabb5046a7 fuzz: Avoid timeout and bloat in fuzz targets (MarcoFalke)
Pull request description:
If the fuzz input contains invalid data *in a loop*, abort early. This will teach the fuzz engine to look for useful data and avoids bloating the fuzz input folder with useless (repeated) data.
ACKs for top commit:
dergoegge:
utACK fabb5046a7
brunoerg:
crACK fabb5046a7
Tree-SHA512: 26da100d7558ae6fdd5292fb146d8858b2af8f78c546ca2509b9d27b33a33e9462ecb6035de142f9f36dd5de32f8cbad099d6c7a697902d23e1bb621cd27dc88
0420f99f42 Create net_peer_connection unit tests (Jon Atack)
4b834f6499 Allow unit tests to access additional CConnman members (Jon Atack)
34b9ef443b net/rpc: Makes CConnman::GetAddedNodeInfo able to return only non-connected address on request (Sergi Delgado Segura)
94e8882d82 rpc: Prevents adding the same ip more than once when formatted differently (Sergi Delgado Segura)
2574b7e177 net/rpc: Check all resolved addresses in ConnectNode rather than just one (Sergi Delgado Segura)
Pull request description:
## Rationale
Currently, `addnode` has a couple of corner cases that allow it to either connect to the same peer more than once, hence wasting outbound connection slots, or add redundant information to `m_added_nodes`, hence making Bitcoin iterate through useless data on a regular basis.
### Connecting to the same node more than once
In general, connecting to the same node more than once is something we should try to prevent. Currently, this is possible via `addnode` in two different ways:
1. Calling `addnode` more than once in a short time period, using two equivalent but distinct addresses
2. Calling `addnode add` using an IP, and `addnode onetry` after with an address that resolved to the same IP
For the former, the issue boils down to `CConnman::ThreadOpenAddedConnections` calling `CConnman::GetAddedNodeInfo` once, and iterating over the result to open connections (`CConman::OpenNetworkConnection`) on the same loop for all addresses.`CConnman::ConnectNode` only checks a single address, at random, when resolving from a hostname, and uses it to check whether we are already connected to it.
An example to test this would be calling:
```
bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" add
```
And check how it allows us to perform both connections some times, and some times it fails.
The latter boils down to the same issue, but takes advantage of `onetry` bypassing the `CConnman::ThreadOpenAddedConnections` logic and calling `CConnman::OpenNetworkConnection` straightaway. A way to test this would be:
```
bitcoin-cli addnode "127.0.0.1:port" add
bitcoin-cli addnode "localhost:port" onetry
```
### Adding the same peer with two different, yet equivalent, addresses
The current implementation of `addnode` is pretty naive when checking what data is added to `m_added_nodes`. Given the collection stores strings, the checks at `CConnman::AddNode()` basically check wether the exact provided string is already in the collection. If so, the data is rejected, otherwise, it is accepted. However, ips can be formatted in several ways that would bypass those checks.
Two examples would be `127.0.0.1` being equal to `127.1` and `[::1]` being equal to `[0:0:0:0:0:0:0:1]`. Adding any pair of these will be allowed by the rpc command, and both will be reported as connected by `getaddednodeinfo`, given they map to the same `CService`.
This is less severe than the previous issue, since even tough both nodes are reported as connected by `getaddednodeinfo`, there is only a single connection to them (as properly reported by `getpeerinfo`). However, this adds redundant data to `m_added_nodes`, which is undesirable.
### Parametrize `CConnman::GetAddedNodeInfo`
Finally, this PR also parametrizes `CConnman::GetAddedNodeInfo` so it returns either all added nodes info, or only info about the nodes we are **not** connected to. This method is used both for `rpc`, in `getaddednodeinfo`, in which we are reporting all data to the user, so the former applies, and to check what nodes we are not connected to, in `CConnman::ThreadOpenAddedConnections`, in which we are currently returning more data than needed and then actively filtering using `CService.fConnected()`
ACKs for top commit:
jonatack:
re-ACK 0420f99f42
kashifs:
> > tACK [0420f9](0420f99f42)
sr-gi:
> > > tACK [0420f9](0420f99f42)
mzumsande:
Tested ACK 0420f99f42
Tree-SHA512: a3a10e748c12d98d439dfb193c75bc8d9486717cda5f41560f5c0ace1baef523d001d5e7eabac9fa466a9159a30bb925cc1327c2d6c4efb89dcaf54e176d1752
1147e00e59 [validation] change package-fee-too-low, return wtxid(s) and effective feerate (glozow)
10dd9f2441 [test] use CheckPackageMempoolAcceptResult in previous tests (glozow)
3979f1afcb [validation] add TxValidationResult::TX_RECONSIDERABLE, TX_UNKNOWN (glozow)
5c786a026a [refactor] use Wtxid for m_wtxids_fee_calculations (glozow)
Pull request description:
Split off from #26711 (suggested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253). This is part of #27463.
- Add 2 new TxValidationResults
- `TX_RECONSIDERABLE` helps us encode transactions who have failed fee checks that can be bypassed using package validation. This is distinguished from `TX_MEMPOOL_POLICY` so that we re-validate a transaction if and only if it is eligible for package CPFP. In the future, we will have a separate cache for reconsiderable rejects so these transactions don't go in `m_recent_rejects`.
- `TX_UNKNOWN` helps us communicate that we aborted package validation and didn't finish looking at this transaction: it's not valid but it's also not invalid (i.e. don't cache it as a rejected tx)
- Return effective feerate and the wtxids of transactions used to calculate that effective feerate when the error is `TX_SINGLE_FAILURE`. Previously, we would only provide this information if the transaction passed. Now that we have package validation, it's much more helpful to the caller to know how the failing feerate was calculated. This can also be used to improve our submitpackage RPC result (which is currently a bit unhelpful when things fail).
- Use the newly added `CheckPackageMempoolAcceptResult` for existing package validation tests. This increases test coverage and helps test the changes made in this PR.
ACKs for top commit:
instagibbs:
reACK 1147e00e59
achow101:
ACK 1147e00e59
murchandamus:
reACK 1147e00e59
ismaelsadeeq:
ACK 1147e00e59
Tree-SHA512: ac1cd73c2b487a1b99d329875d39d8107c91345a5b0b241d54a6a4de67faf11be69a2721cc732c503024a9cca381dac33d61e187957279e3c82653bea118ba91
df69b22f2e doc: improve documentation around connection limit maximums (Amiti Uttarwar)
adc171edf4 scripted-diff: Rename connection limit variables (Amiti Uttarwar)
e9fd9c0225 net: add m_max_inbound to connman (Amiti Uttarwar)
c25e0e0555 net, refactor: move calculations for connection type limits into connman (Amiti Uttarwar)
Pull request description:
This is joint work with amitiuttarwar.
This has the first few commits of #28463. It is not strictly a prerequisite for that, but has changes that in our opinion make sense on their own.
It improves the handling of maximum numbers for different connection types (that are set during init and don’t change after) by:
* moving all calculations into one place, `CConnMan::Init()`. Before, they were dispersed between `Init`, `CConnman::Init` and other parts of `CConnman`, resulting in some duplicated test code.
* removing the possibility of having a negative maximum of inbound connections, which is hard to argue about
* renaming of variables and doc improvements
ACKs for top commit:
amitiuttarwar:
co-author review ACK df69b22f2e
naumenkogs:
ACK df69b22f2e
achow101:
ACK df69b22f2e
Tree-SHA512: 913d56136bc1df739978de50db67302f88bac2a9d34748ae96763288d97093e998fc0f94f9b6eff12867712d7e86225af6128f4170bf2b5b8ab76f024870a22c
c1144f0076 tests: Reset node context members on ~BasicTestingSetup (TheCharlatan)
9759af17ff shutdown: Destroy kernel last (TheCharlatan)
Pull request description:
The destruction/resetting of node context members in the tests should roughly follow the behavior of the `Shutdown` function in `init.cpp`.
This was originally requested by MarcoFalke in this [comment](https://github.com/bitcoin/bitcoin/pull/25065#discussion_r890161249) in response to the [original pull request](https://github.com/bitcoin/bitcoin/pull/25065) introducing the `kernel::Context`.
ACKs for top commit:
maflcko:
ACK c1144f0076 🗣
achow101:
ACK c1144f0076
ryanofsky:
Code review ACK c1144f0076. No code changes since last review, just updated commits and descriptions
Tree-SHA512: 819bb85ff82a5c6c60e429674d5684f3692fe9062500d00a87b361cc59e6bda145be21b5a4466dee6791faed910cbde4d26baab325bf6daa1813af13a63588ff
af0fca530e netbase: use reliable send() during SOCKS5 handshake (Vasil Dimov)
1b19d1117c sock: change Sock::SendComplete() to take Span (Vasil Dimov)
Pull request description:
The `Socks5()` function which does the SOCKS5 handshake with the SOCKS5 proxy sends bytes to the socket without retrying partial writes.
`send(2)` may write only part of the provided data and return. In this case the caller is responsible for retrying the operation with the remaining data. Change `Socks5()` to do that. There is already a method `Sock::SendComplete()` which does exactly that, so use it in `Socks5()`.
A minor complication for this PR is that `Sock::SendComplete()` takes `std::string` argument whereas `Socks5()` has `std::vector<uint8_t>`. Thus the necessity for the first commit. It is possible to do also in other ways - convert the data in `Socks5()` to `std::string` or have just one `Sock::SendComplete()` that takes `void*` and change the callers to pass `str.data(), str.size()` or `vec.data(), vec.size()`.
This came up while testing https://github.com/bitcoin/bitcoin/pull/27375.
ACKs for top commit:
achow101:
ACK af0fca530e
jonatack:
ACK af0fca530e
pinheadmz:
ACK af0fca530e
Tree-SHA512: 1d4a53d0628f7607378038ac56dc3b8624ce9322b034c9547a0c3ce052eafb4b18213f258aa3b57bcb4d990a5e0548a37ec70af2bd55f6e8e6399936f1ce047a
With subpackage evaluation and de-duplication, it's not always the
entire package that is used in CheckFeerate. To be more helpful to the
caller, specify which transactions were included in the evaluation and
what the feerate was.
Instead of PCKG_POLICY (which is supposed to be for package-wide
errors), use PCKG_TX.
fa7ba92630 fuzz: Avoid utxo_total_supply timeout (MarcoFalke)
Pull request description:
Looks like this still may take a long time to run large fuzz inputs. Thus, reduce it further, but still allow it to catch the regression, if re-introduced:
```diff
diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp
index f949655909..4bdd15c5ee 100644
--- a/src/consensus/tx_check.cpp
+++ b/src/consensus/tx_check.cpp
@@ -40,7 +40,7 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
std::set<COutPoint> vInOutPoints;
for (const auto& txin : tx.vin) {
if (!vInOutPoints.insert(txin.prevout).second)
- return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate");
+ {}//return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate");
}
if (tx.IsCoinBase())
```
This is the second take, see https://github.com/bitcoin/bitcoin/pull/27780. If in the future it still times out, I think the fuzz test can just be removed.
Example input:
```
JREROy5pcnAgQyw7IC4ODg4ODg4ODg4O0dEODg4ODg4ZDg4ODg4ODg4ODg7RDg4ODg4ODg4O0dEODg4ODg4ODg4ODg7R0Q4ODg4ODg4ODtHRDg4ODtHR0dEODg4O0dEODg7R0Q4ODg4ODg4ODtHRDg4ODg4ODg4ODg4O0dEODg4ODg4ODg7R0Q4ODg7R0Q4O0dEODg4ODg4ODg4ODg7R0Q4ODg4ODtHRDg4ODtHR
ACKs for top commit:
dergoegge:
ACK fa7ba92630
brunoerg:
utACK fa7ba92630
Tree-SHA512: 154a4895834babede6ce7b775562a7026637af1097e53e55676e92f6cf966ae0c092300ebf7e51a397eebd11f7b41d020586663e781f70d084efda1c0fe851b4
d9cc99d04e [test] MiniMiner::Linearize and manual construction (glozow)
dfd6a3788c [refactor] unify fee amounts in miniminer_tests (glozow)
f4b1b24a3b [MiniMiner] track inclusion order and add Linearize() function (glozow)
004075963f [test] add case for MiniMiner working with negative fee txns (glozow)
fe6332c0ba [MiniMiner] make target_feerate optional (glozow)
5a83f55c96 [MiniMiner] allow manual construction with non-mempool txns (glozow)
e3b2e630b2 [refactor] change MiniMinerMempoolEntry ctor to take values, update includes (glozow)
4aa98b79b2 [lint] update expected boost includes (glozow)
Pull request description:
This is part of #27463. It splits off the `MiniMiner`-specific changes from #26711 for ease of review, as suggested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253.
- Allow using `MiniMiner` on transactions that aren't in the mempool.
- Make `target_feerate` param of `BuildMockTemplate` optional, meaning "don't stop building the template until all the transactions have been selected."
- Add clarification for how this is different from `target_feerate=0` (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1377019133)
- Track the order in which transactions are included in the template to get the "linearization order" of the transactions.
- Tests
Reviewers can take a look at #26711 to see how these functions are used to linearize the `AncestorPackage` there.
ACKs for top commit:
TheCharlatan:
ACK d9cc99d04e
kevkevinpal:
reACK [d9cc99d](d9cc99d04e)
achow101:
re-ACK d9cc99d04e
Tree-SHA512: 32b80064b6679536ac573d674825c5ca0cd6245e49c2fd5eaf260dc535335a57683c74ddd7ce1f249b5b12b2683de4362a7b0f1fc0814c3b3b9f14c682665583
b5a60abe87 MOVEONLY: CleanupTemporaryCoins into its own function (glozow)
10c0a8678c [test util] CreateValidTransaction multi-in/out, configurable feerate, signal BIP125 (glozow)
6ff647a7e0 scripted-diff: rename CheckPackage to IsWellFormedPackage (glozow)
da9aceba21 [refactor] move package checks into helper functions (glozow)
Pull request description:
This is part of #27463. It splits off the more trivial changes from #26711 for ease of review, as requested in https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1786392253.
- Split package sanitization in policy/packages.h into helper functions
- Add some tests for its quirks (https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1340521597)
- Rename `CheckPackage` to `IsPackageWellFormed`
- Improve the `CreateValidTransaction` unit test utility to:
- Configure the target feerate and return the fee paid
- Signal BIP125 on transactions to enable RBF tests
- Allow the specification of multiple inputs and outputs
- Move `CleanupTemporaryCoins` into its own function to be reused later without duplication
ACKs for top commit:
dergoegge:
Code review ACK b5a60abe87
instagibbs:
ACK b5a60abe87
Tree-SHA512: 39d67a5f0041e381f0d0f802a98ccffbff11e44daa3a49611189d6306b03f18613d5ff16c618898d490c97a216753e99e0db231ff14d327f92c17ae4d269cfec
fcb3069fa3 Use CheckPackageMempoolAcceptResult for package evaluation fuzzing (Greg Sanders)
34088d6c9e [test util] CheckPackageMempoolAcceptResult for sanity-checking results (glozow)
651fa404e4 fuzz: tx_pool checks ATMP result invariants (Greg Sanders)
Pull request description:
Poached from https://github.com/bitcoin/bitcoin/pull/26711 since that PR is being split apart, and modified to match current behavior.
ACKs for top commit:
glozow:
reACK fcb3069fa3, only whitespace changes
dergoegge:
ACK fcb3069fa3
Tree-SHA512: abd687e526d8dfc8d65b3a873ece8ca35fdcbd6b0f7b93da6a723ef4e47cf85612de819e6f2b8631bdf897e1aba27cdd86f89b7bd85fc3356e74be275dcdf8cc
9b3da70bd0 [test] DisconnectedBlockTransactions::DynamicMemoryUsage (glozow)
b2d0447964 bugfix: correct DisconnectedBlockTransactions memory usage (stickies-v)
f4254e2098 assume duplicate transactions are not added to `iters_by_txid` (ismaelsadeeq)
29eb219c12 move only: move implementation code to disconnected_transactions.cpp (ismaelsadeeq)
81dfeddea7 refactor: update `MAX_DISCONNECTED_TX_POOL` from kb to bytes (ismaelsadeeq)
Pull request description:
This PR is a follow-up to fix review comments and a bugfix from #28385
The PR
- Updated `DisconnectedBlockTransactions`'s `MAX_DISCONNECTED_TX_POOL` from kb to bytes.
- Moved `DisconnectedBlockTransactions` implementation code to `kernel/disconnected_transactions.cpp`.
- `AddTransactionsFromBlock` now assume duplicate transactions are not passed by asserting after inserting each transaction to `iters_by_txid`.
- Included a Bug fix: In the current master we are underestimating the memory usage of `DisconnectedBlockTransactions`.
* When adding and subtracting `cachedInnerUsage` we call `RecursiveDynamicUsage` with `CTransaction` which invokes this [`RecursiveDynamicUsage(const CTransaction& tx)`](6e721c923c/src/core_memusage.h (L32)) version of `RecursiveDynamicUsage`, the output of that call only account for the memory usage of the inputs and outputs of the `CTransaction`, this omits the memory usage of the `CTransaction` object and the control block.
* This PR fixes this bug by calling `RecursiveDynamicUsage` with `CTransactionRef` when adding and subtracting `cachedInnerUsage` which invokes [`RecursiveDynamicUsage(const std::shared_ptr<X>& p)`](6e721c923c/src/core_memusage.h (L67)) version of `RecursiveDynamicUsage` the output of the calculation accounts for the` CTransaction` object, the control blocks, inputs and outputs memory usage.
* see [comment ](https://github.com/bitcoin/bitcoin/pull/28385#discussion_r1322948452)
- Added test for DisconnectedBlockTransactions memory limit.
ACKs for top commit:
stickies-v:
ACK 9b3da70bd0 - nice work!
BrandonOdiwuor:
re ACK 9b3da70bd0
glozow:
ACK 9b3da70bd0
Tree-SHA512: 69b9595d09f4d0209038f97081d790cea92ccf63efb94e9e372749979fcbe527f7f17a8e454720cedd12021be0c8e11cf99874625d3dafd9ec602b12dbeb4098
Support the creation of a transaction with multiple specified inputs or
outputs. Also accept a target feerate and return the fee paid.
Also, signal BIP125 by default - a subsequent commit needs to RBF
something.
Co-authored-by: Andrew Chow <achow101@gmail.com>
This allows IsSorted() and IsConsistent() to be used by themselves.
IsSorted() with a precomputed set is used so that we don't create this
set multiple times.
for initial partial unit test coverage of these CConnman class methods:
- AddNode()
- ConnectNode()
- GetAddedNodeInfo()
- AlreadyConnectedToAddress()
- ThreadOpenAddedConnections()
and of the GetAddedNodeInfo() call in RPC addnode.
`send(2)` can be interrupted or for another reason it may not fully
complete sending all the bytes. We should be ready to retry the send
with the remaining bytes. This is what `Sock::SendComplete()` does,
thus use it in `Socks5()`.
Since `Sock::SendComplete()` takes a `CThreadInterrupt` argument,
change also the recv part of `Socks5()` to use `CThreadInterrupt`
instead of a boolean.
Easier reviewed with `git show -b` (ignore white-space changes).
99990194ce Remove WithParams serialization helper (MarcoFalke)
ffffb4af83 scripted-diff: Use ser params operator (MarcoFalke)
fae9054793 test: Use SER_PARAMS_OPFUNC in serialize_tests.cpp (MarcoFalke)
Pull request description:
Every serialization parameter struct already has the `SER_PARAMS_OPFUNC`, except for one in the tests.
For consistency, and to remove verbose code, convert the test to `SER_PARAMS_OPFUNC`, and use it everywhere, then remove the `WithParams` helper.
ACKs for top commit:
ajtowns:
reACK 99990194ce
TheCharlatan:
Re-ACK 99990194ce
Tree-SHA512: be9cae4225a502486fe8d552aaf4b2cd2904a9f73cce9d931c6b7c757594ff1982fcc2c30d00d012cd12b0a9531fd609f8bcd7c94b811e965ac087eb8a3589d3
that are otherwise private:
- CConnman::m_nodes
- CConnman::ConnectNodes()
- CConnman::AlreadyConnectedToAddress()
and update the #include headers per iwyu.
`CConnman::GetAddedNodeInfo` is used both to get a list of addresses to manually connect to
in `CConnman::ThreadOpenAddedConnections`, and to report about manually added connections in
`getaddednodeinfo`. In both cases, all addresses added to `m_added_nodes` are returned, however
the nodes we are already connected to are only relevant to the latter, in the former they are
actively discarded.
Parametrizes `CConnman::GetAddedNodeInfo` so we can ask for only addresses we are not connected to,
to avoid passing useless information around.
fb3e812277 p2p: return `CSubNet` in `LookupSubNet` (brunoerg)
Pull request description:
Analyzing the usage of `LookupSubNet`, noticed that most cases uses check if the subnet is valid by calling `subnet.IsValid()`, and the boolean returned by `LookupSubNet` hasn't been used so much, see:
29d540b7ad/src/httpserver.cpp (L172-L174)29d540b7ad/src/net_permissions.cpp (L114-L116)
It makes sense to return `CSubNet` instead of `bool`.
ACKs for top commit:
achow101:
ACK fb3e812277
vasild:
ACK fb3e812277
theStack:
Code-review ACK fb3e812277
stickies-v:
Concept ACK, but Approach ~0 (for now). Reviewed the code (fb3e812277) and it all looks good to me.
Tree-SHA512: ba50d6bd5d58dfdbe1ce1faebd80dd8cf8c92ac53ef33519860b83399afffab482d5658cb6921b849d7a3df6d5cea911412850e08f3f4e27f7af510fbde4b254
940a49978c Use type-safe txid types in orphanage (dergoegge)
ed70e65016 Introduce types for txids & wtxids (dergoegge)
cdb14d79e8 [net processing] Use HasWitness over comparing (w)txids (dergoegge)
Pull request description:
We currently have two different identifiers for transactions: `txid` (refering to the hash of a transaction without witness data) and `wtxid` (referring to the hash of a transaction including witness data). Both are typed as `uint256` which could lead to type-safety bugs in which one transaction identifier type is passed where the other would be expected.
This PR introduces explicit `Txid` and `Wtxid` types that (if used) would cause compilation errors for such type confusion bugs.
(Only the orphanage is converted to use these types in this PR)
ACKs for top commit:
achow101:
ACK 940a49978c
stickies-v:
ACK 940a49978c
hebasto:
ACK 940a49978c, I have reviewed the code and it looks OK.
instagibbs:
re-ACK 940a49978c
BrandonOdiwuor:
re-ACK 940a49978c
glozow:
reACK 940a49978c
Tree-SHA512: 55298d1c2bb82b7a6995e96e554571c22eaf4a89fb2a4d7a236d70e0f625e8cca62ff2490e1c179c47bd93153fe6527b56870198f026f5ee7753d64d7a424c92
The legacy serialization was vulnerable to maleation and is fixed by
adopting the same serialization procedure as was already in use for
MuHash.
This also includes necessary test fixes where the hash_serialized2 was
hardcoded as well as correction of the regtest chainparams.
Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>
dd4dcbd4cd [fuzz] Delete i2p target (dergoegge)
Pull request description:
closes #28665
The target is buggy and doesn't reach basic coverage.
ACKs for top commit:
maflcko:
lgtm ACK dd4dcbd4cd
glozow:
ACK dd4dcbd4cd, agree it's better to delete this test until somebody wants to write a better one
Tree-SHA512: b6ca6cad1773b1ceb6e5ac0fd501ea615f66507ef811745799deaaa4460f1700d96ae03cf55b740a96ed8cd2283b3d6738cd580ba97f2af619197d6c4414ca21
Also add test to make sure this doesn't get broken in the future.
This was breaking vector<bool> serialization in multiprocess code because
template current deduction guides would make it appear like vector<bool> could
be converted to a span, but then the actual conversion to span would fail.
5c8e15c451 i2p: destroy the session if we get an unexpected error from the I2P router (Vasil Dimov)
762404a68c i2p: also sleep after errors in Accept() (Vasil Dimov)
Pull request description:
### Background
In the `i2p::sam::Session` class:
`Listen()` does:
* if the session is not created yet
* create the control socket and on it:
* `HELLO`
* `SESSION CREATE ID=sessid`
* leave the control socked opened
* create a new socket and on it:
* `HELLO`
* `STREAM ACCEPT ID=sessid`
* read reply (`STREAM STATUS`), `Listen()` only succeeds if it contains `RESULT=OK`
Then a wait starts, for a peer to connect. When connected,
`Accept()` does:
* on the socket from `STREAM ACCEPT` from `Listen()`: read the Base64 identification of the connecting peer
### Problem
The I2P router may be in such a state that this happens in a quick succession (many times per second, see https://github.com/bitcoin/bitcoin/issues/22759#issuecomment-1609907115): `Listen()`-succeeds, `Accept()`-fails.
`Accept()` fails because the I2P router sends something that is not Base64 on the socket: `STREAM STATUS RESULT=I2P_ERROR MESSAGE="Session was closed"`
We only sleep after failed `Listen()` because the assumption was that if `Accept()` fails then the next `Listen()` will also fail.
### Solution
Avoid filling the log with "Error accepting:" messages and sleep also after a failed `Accept()`.
### Extra changes
* Reset the error waiting time after one successful connection. Otherwise the timer will remain high due to problems that have been solved long time in the past.
* Increment the wait time less aggressively.
* Handle the unexpected "Session was closed" message more gracefully (don't log stupid messages like `Cannot decode Base64: "STREAM STATUS...`) and destroy the session right way.
ACKs for top commit:
achow101:
ACK 5c8e15c451
jonatack:
re-ACK 5c8e15c451
Tree-SHA512: 1d47958c50eeae9eefcb668b8539fd092adead93328e4bf3355267819304b99ab41cbe1b5dbedbc3452c2bc389dc8330c0e27eb5ccb880e33dc46930a1592885
