| Age | Commit message (Collapse) | Author |
|---|
|
tests methods that use it
61560d5e939034e1a94d95cdc5c498095ab4fddb test: makes timeout a forced named argument in tests methods that use it (Sergi Delgado Segura)
Pull request description:
This makes calls to such methods more explicit and less error-prone.
Motivated by https://github.com/bitcoin/bitcoin/pull/29736#discussion_r1540654057
ACKs for top commit:
maflcko:
lgtm ACK 61560d5e939034e1a94d95cdc5c498095ab4fddb
brunoerg:
ACK 61560d5e939034e1a94d95cdc5c498095ab4fddb
BrandonOdiwuor:
crACK 61560d5e939034e1a94d95cdc5c498095ab4fddb
AngusP:
ACK 61560d5e939034e1a94d95cdc5c498095ab4fddb
stratospher:
tested ACK 61560d5.
Tree-SHA512: 8d6ec3fe1076c868ddbd3050f3c242dbd83cc123f560db3d3b0ed74968e6050dc9ebf4e7c716af9cc1b290c97d736c2fc2ac936b0b69ebdbceed934dae7d55d9
|
|
p2p_node_network_limited.py
2eb5175de87c798af328de3f2147aac7879eaa10 test: fix StopIteration exception in p2p_node_network_limited.py (furszy)
Pull request description:
Fixes #29731
The `next()` call throws an exception if the default parameter is omitted and the iterator is exhausted.
Fix it by providing a default value.
The failure can be tested by commenting out lines 90 and 91 in the test (the `self.connect_nodes(2, 0)`). Since there is no connection, the node in question retrieves a single element in the 'getchaintips()' call. This scenario without the fix, aborts the test right away, throwing an `StopIteration` exception, and with the fix, the test properly waits until the timeout (`wait_until()` call).
ACKs for top commit:
maflcko:
review ACK 2eb5175de87c798af328de3f2147aac7879eaa10
brunoerg:
crACK 2eb5175de87c798af328de3f2147aac7879eaa10
BrandonOdiwuor:
crACK 2eb5175de87c798af328de3f2147aac7879eaa10
tdb3:
Tested ACK for 2eb5175de87c798af328de3f2147aac7879eaa10.
Tree-SHA512: b0873eb4d3334146fd250cd2cd23add3e744877033c8bfa4eb8dff36633100604adf49dd7846856ddfa88c9768663f095db705c00eef3641618df8fc13f8c2c5
|
|
601edd8ee8810b5c6b2184ce8d7f7b45e70913cf ci: use codespell 2.2.6 (fanquake)
52fa0d285f4e8109ebdd8b1e43c897f6bfaa8e65 doc: fix some typos (crazeteam)
b5ed13a2408a141d737341137b26b2f0249c8167 doc: Fix typos (RoboSchmied)
Pull request description:
Combines the recent PRs to fix typos so they can be merged.
ACKs for top commit:
brunoerg:
crACK 601edd8ee8810b5c6b2184ce8d7f7b45e70913cf
tdb3:
crACK 601edd8ee8810b5c6b2184ce8d7f7b45e70913cf
kristapsk:
cr utACK 601edd8ee8810b5c6b2184ce8d7f7b45e70913cf
Tree-SHA512: d054b1dad1336d6b9291cc5d5252d4debf6424a993d4edd6a97d7c15055a7fc48a333d30967f72e7dc9c6c1d9a9038ca8bb5e219c529f4c2365ea48404a508d0
|
|
`gethdkeys` RPCs for adding new automatically generated descriptors
746b6d88395607abbd3c13bbdcdd4ca83e9bc9e4 test: Add test for createwalletdescriptor (Ava Chow)
2402b6306215a9ee8d5f4068ea81f4e7f324adeb wallet: Test upgrade of pre-taproot wallet to have tr() descriptors (Ava Chow)
460ae1bf67c0051033c1802d44787d173abb9248 wallet, rpc: Add createwalletdescriptor RPC (Ava Chow)
8e1a475062e62321e58a0624385cc3fa0885aa12 wallet: Be able to retrieve single key from descriptors (Ava Chow)
85b1fb19dd3a3f3c68da1c5e60a6eb911e1119a6 wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors (Ava Chow)
73926f2d31b61ff78d5f0c8f9b5e3130fb1f9620 wallet, descspkm: Refactor wallet descriptor generation to standalone func (Andrew Chow)
54e74f46ea10e479be682750c1279165f29bb2f4 wallet: Refactor function for single DescSPKM setup (Andrew Chow)
3b09d0eb7f2c1d6ebdab73d18db28e5bf7d74f18 tests: Test for gethdkeys (Ava Chow)
5febe28c9e131fb93fac9c35f80c42759654f150 wallet, rpc: Add gethdkeys RPC (Ava Chow)
66632e5c24c1b59afef1e89b562fbd0117ab6ef5 wallet: Add IsActiveScriptPubKeyMan (Ava Chow)
fa6a259985b61235ebc21eae2a76014cc9437d5f desc spkm: Add functions to retrieve specific private keys (Ava Chow)
fe67841464cc0f970a1c233caba92cb78e9c78dc descriptor: Be able to get the pubkeys involved in a descriptor (Ava Chow)
ef6745879d87cdb6f1061337867a689167e965a1 key: Add constructor for CExtKey that takes CExtPubKey and CKey (Ava Chow)
Pull request description:
This PR adds a `createwalletdescriptor` RPC which allows users to add new automatically generated descriptors to their wallet, e.g. to upgrade a 0.21.x wallet to contain a taproot descriptor. This RPC takes 3 arguments: the output type to create a descriptor for, whether the descriptor will be internal or external, and the HD key to use if the user wishes to use a specific key. The HD key is an optional parameter. If it is not specified, the wallet will use the key shared by the active descriptors, if they are all single key. For most users in the expected upgrade scenario, this should be sufficient. In more advanced cases, the user must specify the HD key to use.
Currently, specified HD keys must already exist in the wallet. To make it easier for the user to know, `gethdkeys` is also added to list out the HD keys in use by all of the descriptors in the wallet. This will include all HD keys, whether we have the private key, for it, which descriptors use it and their activeness, and optionally the extended private key. In this way, users with more complex wallets will be still be able to get HD keys from their wallet for use in other scenarios, and if they want to use `createwalletdescriptor`, they can easily get the keys that they can specify to it.
See also https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1866961865
ACKs for top commit:
Sjors:
re-utACK 746b6d88395607abbd3c13bbdcdd4ca83e9bc9e4
furszy:
ACK 746b6d8
ryanofsky:
Code review ACK 746b6d88395607abbd3c13bbdcdd4ca83e9bc9e4, and this looks ready to merge. There were various suggested changes since last review where main change seems to be switching `gethdkeys` output to use normalized descriptors (removing hardened path components).
Tree-SHA512: f2849101e6fbf1f59cb031eaaaee97af5b1ae92aaab54c5716940d210f08ab4fc952df2725b636596cd5747b8f5beb1a7a533425bc10d09da02659473516fbda
|
|
The `next()` call throws an exception if the default parameter is omitted and the iterator is exhausted.
Fix it by providing a default value.
The failure can be tested by commenting out lines 90 and 91 in the test (the `self.connect_nodes(2, 0)``).
Since there is no connection, the node in question retrieves a single element in the 'getchaintips()' call.
This scenario without the fix, aborts the test right away, throwing an StopIteration exception, and with
the fix, the test properly waits until the timeout (wait_until() call).
|
|
transactions
5952292133d6cc889f51ae771f2e0557311e1efe wallet, rpc: show mempool conflicts in `gettransaction` result (ishaanam)
54e07ee22ff16fc68583ade0d2b8ffffc81d444a wallet: track mempool conflicts (ishaanam)
d64922b5903e5ffc8d2ce0e6761f99f173b60800 wallet refactor: use CWalletTx member functions to determine tx state (ishaanam)
ffe5ff1fb622a8da11b66289e1b778e45e449811 scripted-diff: wallet: s/TxStateConflicted/TxStateBlockConflicted (ishaanam)
180973a94180f9849bf7cb0dab7c9177a942efb8 test: Add tests for wallet mempool conflicts (ishaanam)
Pull request description:
The `mempool_conflicts` variable is added to `CWalletTx`, it is a set of txids of txs in the mempool conflicting with the wallet tx or a wallet tx's parent. This PR only changes how mempool-conflicted txs are dealt with in memory.
`IsSpent` now returns false for an output being spent by a mempool conflicted transaction where it previously returned true.
A txid is added to `mempool_conflicts` during `transactionAddedToMempool`. A txid is removed from `mempool_conflicts` during `transactionRemovedFromMempool`.
This PR also adds a `mempoolconflicts` field to the `gettransaction` wallet RPC result.
Builds on #27145
Second attempt at #18600
ACKs for top commit:
achow101:
ACK 5952292133d6cc889f51ae771f2e0557311e1efe
ryanofsky:
Code review ACK 5952292133d6cc889f51ae771f2e0557311e1efe. Just small suggested changes since last review
furszy:
ACK 59522921
Tree-SHA512: 615779606723dbb6c2e302681d8e58ae2052ffee52d721ee0389746ddbbcf4b4c4afacf01ddf42b6405bc6f883520524186a955bf6b628fe9b3ae54cffc56a29
|
|
This makes calls to such methods more explicit and less error prone
|
|
80fa7da21c470302165c47cc4a6a62fb44f997ef test: Refactor subtree exclusion in lint tests (Brandon Odiwuor)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/17413
Refactor subtree exclusion in lint tests to one place
Second attempt after PR: https://github.com/bitcoin/bitcoin/pull/24435
ACKs for top commit:
fjahr:
re-ACK 80fa7da21c470302165c47cc4a6a62fb44f997ef
maflcko:
lgtm ACK 80fa7da21c470302165c47cc4a6a62fb44f997ef
davidgumberg:
ACK https://github.com/bitcoin/bitcoin/commit/80fa7da21c470302165c47cc4a6a62fb44f997ef
Tree-SHA512: deff7457dd19ca5ea440d3d53feae047e8863b9ddeb6494a3c94605a5d16edc91db8f99a435b4fab2ef89aedee42439562be006da647fb85bbf3def903a3ce50
|
|
Signed-off-by: crazeteam <lilujing@outlook.com>
|
|
|
|
`wallet_keypool_topup`
a8bfc3dea1d986b458202bf5e49cf1944392d676 test: add coverage for bech32m in `wallet_keypool_topup` (brunoerg)
Pull request description:
0dcac51049cdd924a50d62629757effc8d542046 added coverage for all keypool addresses types in `wallet_keypool_topup` (4y ago). Now we have bech23m, so this PR adds it.
ACKs for top commit:
achow101:
ACK a8bfc3dea1d986b458202bf5e49cf1944392d676
marcofleon:
ACK a8bfc3dea1d986b458202bf5e49cf1944392d676. Definitely a more straightfoward addition to the test. Reviewed the code, built the PR branch and ran all functional tests without issues.
furszy:
utACK a8bfc3dea
Tree-SHA512: aa830b723a7a54b23744f9fb3cf5214452c4ffc8e3bbe0e8bd980bdf902e61c3dd2fd57361b82c5c0c5224aa0774158daf34b6b2188edda0a971f82111976051
|
|
fa1146d01b148dd60fcada36a3b37ed37532ce2b lint: Fix COMMIT_RANGE issues (MarcoFalke)
Pull request description:
`COMMIT_RANGE` has problems on forks or local branches:
* When `LOCAL_BRANCH` is set, it assumes the presence of a `master` branch, and that the `master` branch is up-to-date. Both of which may be false. (See also discussion in https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1504226422)
* When `COMMIT_RANGE` isn't set in `lint-git-commit-check.py`, and `--prev-commits` isn't set either, it has the same (broken) assumptions.
Fix all issues by simply assuming a merge commit exists. This allows to drop `LOCAL_BRANCH`. It also allows to drop `SKIP_EMPTY_NOT_A_PR`, because scripts will already skip an empty range. Finally, it allows to drop `--prev-commits n`, because one can simply say `COMMIT_RANGE='HEAD~n..HEAD'` to achieve the same.
ACKs for top commit:
Sjors:
tACK fa1146d01b148dd60fcada36a3b37ed37532ce2b
Tree-SHA512: f1477a38267fd4fdb8d396211a5d6bed5f418798c7edaba43487957aaf726cd45244ccf15187b3dd896d398fa1df3fe0a37323e49cf44d60a2018786ed41e5ba
|
|
The addpeeraddress calls can fail due to collisions. As we are using a
deteministic addrman, they won't fail with the current bucket/position
calculation. However, if the calculation is changed, they might collide
and fail silently causing tests using `seed_addrman()` to fail.
Assert that the addpeeraddress calls are successful.
|
|
consistent
824f47294a309ba8e58ba8d1da0af15d8d828f43 node: Use log levels in noui_ThreadSafeMessageBox (TheCharlatan)
ddc7872c08b7ddf9b1e83abdb97c21303f4a9172 node: Make translations of fatal errors consistent (TheCharlatan)
Pull request description:
The extra `bilingual_str` argument of the fatal error notifications and `node::AbortNode()` is often unused and when used usually contains the same string as the message argument. It also seems to be confusing, since it is not consistently used for errors requiring user action. For example some assumeutxo fatal errors require the user to do something, but are not translated.
So simplify the fatal error and abort node interfaces by only passing a translated string. This slightly changes the fatal errors displayed to the user.
ACKs for top commit:
stickies-v:
re-ACK 824f47294a309ba8e58ba8d1da0af15d8d828f43
maflcko:
ACK 824f47294a309ba8e58ba8d1da0af15d8d828f43 🔎
achow101:
ACK 824f47294a309ba8e58ba8d1da0af15d8d828f43
hebasto:
re-ACK 824f47294a309ba8e58ba8d1da0af15d8d828f43.
Tree-SHA512: 2868ee7b045fe7f3ac582ce5039141b398480b7627734976201dafaaef7544b8461635a7292fee4a7f32ff1bfc26f9bd4d0c292dca424ba42fb7fc4483d7ce8d
|
|
99954f914f031c80aa53daa367fc049c4c55bdf3 test: fix test to ensure hidden RPC is present in detailed help (stratospher)
0d01f6f0c6e53c9765f84e0616ab46b83923a6ad test: remove unused mocktime in test_addpeeraddress (0xb10c)
6205466512d4b94d1e507a77ab2151425790d29f rpc: "addpeeraddress tried" return error on failure (0xb10c)
Pull request description:
When trying to add an address to the IP address manager tried table, it's first added to the new table and then moved to the tried table. Previously, adding a conflicting address to the address manager's tried table with test-only `addpeeraddress tried=true` RPC would return `{ "success": true }`. However, the address would not be added to the tried table, but would remain in the new table. This caused, e.g., issue #28964.
This is fixed by new returning `{ "success": false, "error": "..." }` for failed tried table additions. Since the address remaining in the new table can't be removed (the address manager interface does not support removing addresses at the moment and adding this seems to be a bigger effort), an error message is returned. This indicates to a user why the RPC failed and allows accounting for the extra address in the new table. This is done in the functional test for the `getrawaddrman` RPC.
Fixes #28964
ACKs for top commit:
achow101:
ACK 99954f914f031c80aa53daa367fc049c4c55bdf3
stratospher:
reACK 99954f9. 🚀
brunoerg:
utACK 99954f914f031c80aa53daa367fc049c4c55bdf3
Tree-SHA512: 2f1299410c0582ebc2071271ba789a8abed905f9a510821f77afbcf2a555ec31397578ea55cbcd162fb828be27afedd3246c7b13ad8883f2f745bb8e04364a76
|
|
The test requires that limited nodes are not peered with when
the node's system time exceeds ~ 24h of the node's chaintip
timestamp, as per PeerManagerImpl::GetDesirableServiceFlags.
By patching this test to modify the timestamp of the chaintip as
opposed to mocking the node's system time, we make it resilient
to future commits where the node raises a warning if it detects
its system time is too much out of sync with its outbound peers.
See https://github.com/bitcoin/bitcoin/pull/29623
|
|
|
|
The extra `bilingual_str` argument of the fatal error notifications and
`node::AbortNode()` is often unused and when used usually contains the
same string as the message argument. It also seems to be confusing,
since it is not consistently used for errors requiring user action. For
example some assumeutxo fatal errors require the user to do something,
but are not translated.
So simplify the fatal error and abort node interfaces by only passing a
translated string. This slightly changes the fatal errors displayed to
the user.
Also de-duplicate the abort error log since it is repeated in noui.cpp.
|
|
|
|
|
|
|
|
|
|
Behavior changes are:
- if a tx has a mempool conflict, the wallet will not attempt to
rebroadcast it
- if a txo is spent by a mempool-conflicted tx, that txo is no
longer considered spent
|
|
values
9d9a7458a2570f7db56ab626b22010591089c312 assumeutxo: Remove BLOCK_ASSUMED_VALID flag (Ryan Ofsky)
ef174e9ed21c08f38e5d4b537b6decfd1f646db9 test: assumeutxo snapshot block CheckBlockIndex crash test (Ryan Ofsky)
0391458d767b842a7925785a7053400c0e1cb55a test: assumeutxo stale block CheckBlockIndex crash test (Ryan Ofsky)
ef29c8b662309a438121a83f27fd7bdd1779700c assumeutxo: Get rid of faked nTx and nChainTx values (Ryan Ofsky)
9b97d5bbf980d657a277c85d113c2ae3e870e0ec doc: Improve comments describing setBlockIndexCandidates checks (Ryan Ofsky)
0fd915ee6bef63bb360ccc5c039a3c11676c38e3 validation: Check GuessVerificationProgress is not called with disconnected block (Ryan Ofsky)
63e8fc912c21a2f5b47e8eab10fb13c604afed85 ci: add getchaintxstats ubsan suppressions (Ryan Ofsky)
f252e687ec94b6ccafb5bc44b7df3daeb473fdea assumeutxo test: Add RPC test for fake nTx and nChainTx values (Ryan Ofsky)
Pull request description:
The `PopulateAndValidateSnapshot` function introduced in f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake `nTx` and `nChainTx` values that can show up in RPC results (https://github.com/bitcoin/bitcoin/issues/29328) and make `CBlockIndex` state hard to reason about, because it is difficult to know whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the values are unknown, instead of faking them. Also drop no-longer needed `BLOCK_ASSUMED_VALID` flag.
Dropping the faked values also fixes assert failures in the `CheckBlockIndex` `(pindex->nChainTx == pindex->nTx + prev_chain_tx)` check that could happen previously if forked or out-of-order blocks before the snapshot got submitted while the snapshot was being validated. The PR includes two commits adding tests for these failures and describing them in detail.
Compatibility note: This change could cause new `-checkblockindex` failures if a snapshot was loaded by a previous version of Bitcoin Core and not fully validated, because fake `nTx` values will have been saved to the block index. It would be pretty easy to avoid these failures by adding some compatibility code to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake (when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a little simpler not to worry about being compatible in this case.
ACKs for top commit:
Sjors:
re-ACK 9d9a7458a2570f7db56ab626b22010591089c312
achow101:
ACK 9d9a7458a2570f7db56ab626b22010591089c312
mzumsande:
Tested ACK 9d9a7458a2570f7db56ab626b22010591089c312
maflcko:
ACK 9d9a7458a2570f7db56ab626b22010591089c312 🎯
Tree-SHA512: b1e1e2731ec36be30d5f5914042517219378fc31486674030c29d9c7488ed83fb60ba7095600f469dc32f0d8ba79c49ff7706303006507654e1762f26ee416e0
|
|
is already there
dfcef536d0e6c40e98dce35ae7af6e3e4a2595cd blockstorage: do not flush block to disk if it is already there (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/2039
When reindexing from flat-file block storage there is no need to write anything back to disk, since the block data is already there. This PR skips flushing to disk those blocks that already have a known position in the datastore. Skipping this means that users can write-protect the `blk` files on disk which may be useful for security or even safely sharing that data between multiple bitcoind instances.
`FindBlockPos()` may also flush the undo data file, but again this is skipped if the corresponding block position is known, like during the initial stage of a reindex when block data is being indexed. Once the block index is complete the validation mechanism will call `ConnectBlock()` which will save undo data at that time.
The call stack looks like this:
```
init()
ThreadImport() <-- process fReindex flag
LoadExternalBlockFile()
AcceptBlock()
SaveBlockToDisk()
FindBlockPos()
FlushBlockFile() <-- unnecessary if block is already on disk
```
A larger refactor of this part of the code was started by mzumsande here: https://github.com/mzumsande/bitcoin/tree/202207_refactor_findblockpos including this fix, reviewers can let me know if the changes should be combined.
ACKs for top commit:
sipa:
utACK dfcef536d0e6c40e98dce35ae7af6e3e4a2595cd
mzumsande:
re-ACK dfcef536d0e6c40e98dce35ae7af6e3e4a2595cd
achow101:
ACK dfcef536d0e6c40e98dce35ae7af6e3e4a2595cd
furszy:
Rebase diff ACK dfcef53.
Tree-SHA512: 385c5ac1288b325135398d0ddd3ab788fa98cc0ca19bd2474c74039f2ce70d5088c1d1c9d4dd10aefcbd4c757767ec5805d07ba8cee9289a66f96e6f9eaa5279
|
|
|
|
desirable service flags
2f23987849758537f76df7374d85a7e87b578b61 test: p2p: check limited peers desirability (depending on best block depth) (Sebastian Falbesoner)
c4a67d396d0aa99f658cafe381e39622859eb0be test: p2p: check disconnect due to lack of desirable service flags (Sebastian Falbesoner)
405ac819af1eb0f6cf6d1805cb668f4e8ab4a6f3 test: p2p: support disconnect waiting for `add_outbound_p2p_connection` (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for disconnecting peers which don't offer the desirable service flags in their VERSION message:
https://github.com/bitcoin/bitcoin/blob/5f3a0574c45477288bc678b15f24940486084576/src/net_processing.cpp#L3384-L3389
This check is relevant for the connection types "outbound-full-relay", "block-relay-only" and "addr-fetch" (see `CNode::ExpectServicesFromConn(...)`). Feeler connections always disconnect, which is also tested here.
In lack of finding a proper file where this test would fit in, I created a new one. Happy to take suggestions there.
ACKs for top commit:
davidgumberg:
reACK https://github.com/bitcoin/bitcoin/commit/2f23987849758537f76df7374d85a7e87b578b61
itornaza:
tested ACK 2f23987849758537f76df7374d85a7e87b578b61
fjahr:
re-utACK 2f23987849758537f76df7374d85a7e87b578b61
cbergqvist:
re ACK 2f23987849758537f76df7374d85a7e87b578b61
stratospher:
tested ACK 2f23987. 🚀
Tree-SHA512: cf75d9d4379d0f34fa1e13152e6a8d93cd51b9573466ab3a2fec32dc3e1ac49b174bd1063cae558bc736b111c8a6e7058b1b57a496df56255221bf367d29eb5d
|
|
current check to make sure that detailed help for hidden RPC
is displayed won't work because the assertion isn't sufficient.
Even if unknown RPCs are passed, RPC names would still be present
in node.help().
|
|
Drops the mocktime added in fa4c6836c9366c3cc575cb386a397840d5f1aa57.
Setting the mocktime in test_addpeeraddress() isn't needed
anymore as it doesn't leak into test_getrawaddrman() anymore
(since 2cc8ca19f4185490f30a49516c890b2289fbab71).
test_getrawaddrman() clear's the addrman and sets it's own
mocktime.
|
|
When trying to add an address to the IP address manager tried table,
it's first added to the new table and then moved to the tried table.
Previously, adding a conflicting address to the address manager's
tried table with test-only `addpeeraddress tried=true` RPC would
return `{ "success": true }`. However, the address would not be added
to the tried table, but would remain in the new table. This caused,
e.g., issue 28964.
This is fixed by returning `{ "success": false, "error":
"failed-adding-to-tried" }` for failed tried table additions. Since
the address remaining in the new table can't be removed (the address
manager interface does not support removing addresses at the moment
and adding this seems to be a bigger effort), an error message is
returned. This indicates to a user why the RPC failed and allows
accounting for the extra address in the new table.
Also:
To check the number of addresses in each addrman table,
the addrman checks were re-run and the log output of this check
was asserted. Ideally, logs shouldn't be used as an interface
in automated tests. To avoid asserting the logs, use the getaddrmaninfo
and getrawaddrman RPCs (which weren't implemented when the test was added).
Removing the "getnodeaddress" calls would also remove the addrman checks
from the test, which could reduce the test coverage. To avoid this,
these are kept.
|
|
432a542e271f5b6ecb1c6ea4fa9108ad4b3a5a43 test: fix intermittent failures with test=addrman (Martin Zumsande)
Pull request description:
The `nKey` of the addrman is generated the first time the node is started with an empty `peers.dat`. Therefore, restarting a node or turning it off and on again won't make a previously non-deterministic addrman deterministic.
This could lead to intermittent failures in `feature_asmap.py` and `rpc_net.py`
Fixes #29634
ACKs for top commit:
kevkevinpal:
ACK [432a542](https://github.com/bitcoin/bitcoin/pull/29639/commits/432a542e271f5b6ecb1c6ea4fa9108ad4b3a5a43)
stratospher:
Tested ACK 432a542e271f5b6ecb1c6ea4fa9108ad4b3a5a43.
brunoerg:
crACK 432a542e271f5b6ecb1c6ea4fa9108ad4b3a5a43
0xB10C:
ACK 432a542e271f5b6ecb1c6ea4fa9108ad4b3a5a43
Tree-SHA512: a8e284baeb0be2df7284b8a2792cb9edc9e2d5b877a3b29ab7277ffdb75b17efa58a4d42576441eb493cd518e7c5ffdb05597b27e42b5001cf1a80e78bb04c83
|
|
submitpackage
38f70ba6ac86fb96c60571d2e1f316315c1c73cc RPC: Add maxfeerate and maxburnamount args to submitpackage (Greg Sanders)
Pull request description:
Resolves https://github.com/bitcoin/bitcoin/issues/28949
I couldn't manage to do it very cleanly outside of (sub)package evaluation itself, since it would change the current interface very heavily. Instead I threaded through the max fee argument and used that directly via ATMPArgs. From that perspective, this is somewhat a reversion from https://github.com/bitcoin/bitcoin/pull/19339. In a post-cluster mempool world, these checks could be consolidated to right after the given (ancestor) package is linearized/chunked, by just checking the feerate of the top chunk and rejecting the submission entirely if the top chunk is too high.
The implication here is that subpackages can be submitted to the mempool prior to hitting this new fee-based error condition.
ACKs for top commit:
ismaelsadeeq:
Re-ACK https://github.com/bitcoin/bitcoin/commit/38f70ba6ac86fb96c60571d2e1f316315c1c73cc 👍🏾
glozow:
ACK 38f70ba6ac with some non-blocking nits
murchandamus:
LGTM, code review ACK 38f70ba6ac86fb96c60571d2e1f316315c1c73cc
Tree-SHA512: 38212aa9de25730944cee58b0806a3d37097e42719af8dd7de91ce86bb5d9770b6f7c37354bf418bd8ba571c52947da1dcdbb968bf429dd1dbdf8715315af18f
|
|
Add a test for a CheckBlockIndex crash that would happen before previous
"assumeutxo: Get rid of faked nTx and nChainTx values" commit.
The crash was an assert failure in the (pindex->nChainTx == pindex->nTx +
prev_chain_tx) check that would previously happen if the snapshot block was
submitted after loading the snapshot and downloading a few blocks after the
snapshot. In that case ReceivedBlockTransactions() previously would overwrite
the nChainTx value of the submitted snapshot block with a fake value based on
the previous block, so the (pindex->nChainTx == pindex->nTx + prev_chain_tx)
check would later fail on the first block after the snapshot. This test was
originally posted by Martin Zumsande <mzumsande@gmail.com> in
https://github.com/bitcoin/bitcoin/pull/29370#issuecomment-1974096225
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
|
|
Add a test for a CheckBlockIndex crash that would happen before previous
"assumeutxo: Get rid of faked nTx and nChainTx values" commit.
The crash was an assert failure in the (pindex->nChainTx == pindex->nTx +
prev_chain_tx) check that would previously happen if a snapshot was loaded, and
a block was submitted which forked from the chain before the snapshot block and
after the last downloaded background chain block. This block would not be
marked assumed-valid because it would not be an ancestor of the snapshot, and
it would have nTx set, nChainTx unset, and prev->nChainTx set with a fake
value, so the assert would fail. After the fix, prev->nChainTx is unset instead
of being set to a fake value, so the assert succeeds. This test was originally
posted by maflcko in
https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1918947945
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
|
|
The `PopulateAndValidateSnapshot` function introduced in
f6e2da5fb7c6406c37612c838c998078ea8d2252 from #19806 has been setting fake
`nTx` and `nChainTx` values that can show up in RPC results (see #29328) and
make `CBlockIndex` state hard to reason about, because it is difficult to know
whether the values are real or fake.
Revert to previous behavior of setting `nTx` and `nChainTx` to 0 when the
values are unknown, instead of faking them.
This commit fixes at least two assert failures in the (pindex->nChainTx ==
pindex->nTx + prev_chain_tx) check that would happen previously. Tests for
these failures are added separately in the next two commits.
Compatibility note: This change could result in -checkblockindex failures if a
snapshot was loaded by a previous version of Bitcoin Core and not fully
validated, because fake nTx values will have been saved to the block index. It
would be pretty easy to avoid these failures by adding some compatibility code
to `LoadBlockIndex` and changing `nTx` values from 1 to 0 when they are fake
(when `(pindex->nStatus & BLOCK_VALID_MASK) < BLOCK_VALID_TRANSACTIONS`), but a
little simpler not to worry about being compatible in this case.
|
|
Add ubsan suppressions for integer overflows in the getchaintxstats RPC.
getchainstatstx line "int nTxDiff = pindex->nChainTx - past_block.nChainTx" can
trigger ubsan integer overflows when assumeutxo snapshots are loaded, from
subtracting unsigned values and assigning the result to a signed int.
The overflow behavior probably exists in current code but is hard to trigger
because it would require calling getchainstatstx at the right time with
specific parameters as background blocks are being downloaded. But the overflow
behavior becomes easier to trigger in the upcoming commit removing fake
nChainTx values, so a suppression needs to be added before then for CI to pass.
getchainstatstx should probably be improved separately in another PR to not
need this suppression, and handle edge cases and missing nChainTx values more
carefully.
|
|
The fake values will be removed in an upcoming commit, so it is useful to have
test coverage confirming the change in behavior.
|
|
|
|
5555395c15e896230a55c131fc3cbfd9d116adf8 lint: Use git --no-pager to print any output in one go (MarcoFalke)
fa5729436ca12b20cfa2cd1f0c6f54af7192f0a6 lint: Fix lint-whitespace issues (MarcoFalke)
Pull request description:
The lint check has many issues:
* It uses `COMMIT_RANGE`, which is brittle code, apparently making it harder to run the CI locally, or self-hosted. See https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457739319
* The result depends on `COMMIT_RANGE`, or the number of commits passed to the script, which can cause false negatives or false positives.
* It is based on the diff output, parsing it, and printing it again, which is brittle as well.
* The output does not include line number, making it harder to act on a lint error.
Fix all issues by removing the script and replacing it with a simple call to `git grep -I --line-number ...`.
ACKs for top commit:
TheCharlatan:
Re-ACK 5555395c15e896230a55c131fc3cbfd9d116adf8
Tree-SHA512: 71ea8b6382af064beb72fb17f21a0ae9e9238c97e7fa43c2ec353fd1dd73a7bbd696ba0f0a9f65d1eff7c86cbf6cc104a992cb5450a3d50f122955e835270065
|
|
|
|
bf264e05981e3809715f34f548138d53991db6f2 test: check_mempool_result negative feerate (kevkevin)
Pull request description:
Adds test coverage in `mempool_accept.py` to check if a negative `maxfeerate` is input into `check_mempool_result`
Asserts "Amount out of range" error message and `-3` error code
Motivated by this [comment](https://github.com/bitcoin/bitcoin/pull/29434/files#r1491112250)
ACKs for top commit:
maflcko:
lgtm ACK bf264e05981e3809715f34f548138d53991db6f2
brunoerg:
nice, utACK bf264e05981e3809715f34f548138d53991db6f2
davidgumberg:
Looks great, ACK https://github.com/bitcoin/bitcoin/pull/29459/commits/bf264e05981e3809715f34f548138d53991db6f2
Tree-SHA512: 58931b774cc887c616f2fd91af3ee65cc5db55acd8e2875c76de448c80bd4e020b057c5f4f85556431377f0d0e7553771fb285d1ec20cf64f64ec92a47776b78
|
|
0831b54dfca1b9e728295fff500215da14589fc0 test: simplify test_runner.py (tdb3)
Pull request description:
Implements the simplifications to test_runner.py proposed by sipa in PR #23995.
Remove the num_running variable as it can be implied by the length of the jobs list.
Remove the i variable as it can be implied by the length of the test_results list.
Instead of counting results to determine if finished, make the queue object itself
responsible (by looking at running jobs and jobs left).
ACKs for top commit:
mzumsande:
re-ACK 0831b54
davidgumberg:
reACK https://github.com/bitcoin/bitcoin/commit/0831b54dfca1b9e728295fff500215da14589fc0
marcofleon:
re-ACK 0831b54dfca1b9e728295fff500215da14589fc0
Tree-SHA512: e5473e68d49cd779b29d97635329283ae7195412cb1e92461675715ca7eedb6519a1a93ba28d40ca6f015d270f7bcd3e77cef279d9cd655155ab7805b49638f1
|
|
|
|
1ec6684b08614f593b37e627a0a08e54b19318b6 test: Add test for loadtxoutset when headers are not synced (Fabian Jahr)
2bc1ecfaa9b69a20388e913ec64967de2f506cd3 test: Remove unnecessary sync_blocks in assumeutxo tests (Fabian Jahr)
Pull request description:
It adds a test for the change to `loadtxoutset` made in #29345. Before that change the test doesn't fail right away but times out after 10 minutes.
Also removes a `sync_blocks` call that didn't seem to do anything valuable.
ACKs for top commit:
achow101:
ACK 1ec6684b08614f593b37e627a0a08e54b19318b6
pablomartin4btc:
tACK 1ec6684b08614f593b37e627a0a08e54b19318b6
BrandonOdiwuor:
ACK 1ec6684b08614f593b37e627a0a08e54b19318b6
theStack:
ACK 1ec6684b08614f593b37e627a0a08e54b19318b6
Tree-SHA512: 1337decdf91e4a4f7213fcf8ace1d705e5c1422e0ac3872a59b5be9c33e743850cb8f5f7474750a534952eefd5cfe43fe85a54efb9bc0e47515136a2903676e5
|
|
And thread the feerate value through ProcessNewPackage to
reject individual transactions that exceed the given
feerate. This allows subpackage processing, and is
compatible with future package RBF work.
|
|
-onion
567cec9a05e1261e955535f734826b12341684b6 doc: add release notes and help text for unix sockets (Matthew Zipkin)
bfe51928911daf484ae07deb52a7ff0bcb2526ae test: cover UNIX sockets in feature_proxy.py (Matthew Zipkin)
c65c0d01630b44fa71321ea7ad68d5f9fbb7aefb init: allow UNIX socket path for -proxy and -onion (Matthew Zipkin)
c3bd43142eba77dcf1acd4984e437759f65e237a gui: accomodate unix socket Proxy in updateDefaultProxyNets() (Matthew Zipkin)
a88bf9dedd1d8c1db0a9c8b663dab3e3c2f0f030 i2p: construct Session with Proxy instead of CService (Matthew Zipkin)
d9318a37ec09fe0b002815a7e48710e530620ae2 net: split ConnectToSocket() from ConnectDirectly() for unix sockets (Matthew Zipkin)
ac2ecf3182fb5ad9bcd41540b19382376114d6ee proxy: rename randomize_credentials to m_randomize_credentials (Matthew Zipkin)
a89c3f59dc44eaf4f59912c1accfc0ce5d61933a netbase: extend Proxy class to wrap UNIX socket as well as TCP (Matthew Zipkin)
3a7d6548effa6cd9a4a5413b690c2fd85da4ef65 net: move CreateSock() calls from ConnectNode() to netbase methods (Matthew Zipkin)
74f568cb6fd5c74b7b9bf0ce69876430746a53b1 netbase: allow CreateSock() to create UNIX sockets if supported (Matthew Zipkin)
bae86c8d318d06818aa75a9ebe3db864197f0bc6 netbase: refactor CreateSock() to accept sa_family_t (Matthew Zipkin)
adb3a3e51de205cc69b1a58647c65c04fa6c6362 configure: test for unix domain sockets (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/27252
UNIX domain sockets are a mechanism for inter-process communication that are faster than local TCP ports (because there is no need for TCP overhead) and potentially more secure because access is managed by the filesystem instead of serving an open port on the system.
There has been work on [unix domain sockets before](https://github.com/bitcoin/bitcoin/pull/9979) but for now I just wanted to start on this single use-case which is enabling unix sockets from the client side, specifically connecting to a local Tor proxy (Tor can listen on unix sockets and even enforces strict curent-user-only access permission before binding) configured by `-onion=` or `-proxy=`
I copied the prefix `unix:` usage from Tor. With this patch built locally you can test with your own filesystem path (example):
`tor --SocksPort unix:/Users/matthewzipkin/torsocket/x`
`bitcoind -proxy=unix:/Users/matthewzipkin/torsocket/x`
Prep work for this feature includes:
- Moving where and how we create `sockaddr` and `Sock` to accommodate `AF_UNIX` without disturbing `CService`
- Expanding `Proxy` class to represent either a `CService` or a UNIX socket (by its file path)
Future work:
- Enable UNIX sockets for ZMQ (https://github.com/bitcoin/bitcoin/pull/27679)
- Enable UNIX sockets for I2P SAM proxy (some code is included in this PR but not tested or exposed to user options yet)
- Enable UNIX sockets on windows where supported
- Update Network Proxies dialog in GUI to support UNIX sockets
ACKs for top commit:
Sjors:
re-ACK 567cec9a05e1261e955535f734826b12341684b6
tdb3:
re ACK for 567cec9a05e1261e955535f734826b12341684b6.
achow101:
ACK 567cec9a05e1261e955535f734826b12341684b6
vasild:
ACK 567cec9a05e1261e955535f734826b12341684b6
Tree-SHA512: de81860e56d5de83217a18df4c35297732b4ad491e293a0153d2d02a0bde1d022700a1131279b187ef219651487537354b9d06d10fde56225500c7e257df92c1
|
|
Remove the num_running variable as it can be implied by the
length of the jobs list.
Remove the i variable as it can be implied by the length of the
test_results list.
Instead of counting results to determine if finished, make the
queue object itself responsible (by looking at running jobs and
jobs left).
Originally proposed by @sipa in PR #23995.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
|
|
The nKey of the addrman is generated the first time the node is
started. Therefore, restarting a node or turning it off and on
again won't make a previously non-deterministic addrman
deterministic.
Co-authored-by: 0xb10c <b10c@b10c.me>
|
|
rest_block by reading raw block
e710cefd5701cd33d1e55034b3e37cea78582733 rest: read raw block in rest_block and deserialize for json (Andrew Toth)
95ce0783a6dab325038a64d6c529c9e7816e3072 rpc: read raw block in getblock and deserialize for verbosity > 0 (Andrew Toth)
0865ab8712429761bc69f09d93760f8c6081c99c test: check more details on zmq raw block response (Andrew Toth)
38265cc14e7d646bf27882329d374d42167eb49f zmq: read raw block with ReadRawBlockFromDisk (Andrew Toth)
da338aada7943c392013c36c542af621fbc6edd1 blockstorage: check nPos in ReadRawBlockFromDisk before seeking back (Andrew Toth)
Pull request description:
For the `getblock` endpoint with `verbosity=0`, the `rest_block` REST endpoint for `bin` and `hex`, and zmq `NotifyBlock` we don't have to deserialize the block since we're just sending the raw data. This PR uses `ReadRawBlockFromDisk` instead of `ReadBlockFromDisk` to serve these requests, and only deserializes for `verbosity > 0` and `json` REST requests. See benchmarks in https://github.com/bitcoin/bitcoin/pull/26684.
Benchmarked using ApacheBench. Requesting block 750,000 in binary 10k times on a single core (set `-rest=1` in config):
`ab -n 10000 -c 1 "http://127.0.0.1:8332/rest/block/0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e.bin"`
On master, mean time 15ms.
On this branch, mean time 1ms.
For RPC
```
echo '{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", 0]}' > /tmp/data.json
ab -p /tmp/data.json -n 1000 -c 1 -A user:password "http://127.0.0.1:8332/"
```
On master, mean time 32ms
On this branch, mean time 13ms
ACKs for top commit:
achow101:
re-ACK e710cefd5701cd33d1e55034b3e37cea78582733
Tree-SHA512: 4cea13c7b563b2139d041b1fdcfdb793c8cc688654ae08db07e7ee6b875c5e582b8185db3ae603abbfb06d2164724f29205774620b48c493726b991999af289e
|
|
0a533613fb44207053796fd01a9f4b523a3153d4 docs: add release notes for #27114 (brunoerg)
e6b8f19de9a6d1c477d0bbda18d17794cd81a6f4 test: add coverage for whitelisting manual connections (brunoerg)
c985eb854cc86deb747caea5283c17cf51b6a983 test: add option to speed up tx relay/mempool sync (brunoerg)
66bc6e2d1749f43d7b314aa2784a06af78440170 Accept "in" and "out" flags to -whitelist to allow whitelisting manual connections (Luke Dashjr)
8e06be347c5e14cbe75256eba170e0867f95f360 net_processing: Move extra service flag into InitializeNode (Luke Dashjr)
9133fd69a5cc9a0ab1a06a60d09f1b7e1039018e net: Move `NetPermissionFlags::Implicit` verification to `AddWhitelistPermissionFlags` (Luke Dashjr)
2863d7dddb62d987b3e1c3b8bfad7083f0f774b2 net: store `-whitelist{force}relay` values in `CConnman` (brunoerg)
Pull request description:
Revives #17167. It allows whitelisting manual connections. Fixes #9923
Since there are some PRs/issues around this topic, I'll list some motivations/comments for whitelisting outbound connections from them:
- Speed-up tx relay/mempool sync for testing purposes (my personal motivation for this) - In #26970, theStack pointed out that we whitelist peers to speed up tx relay for fast mempool synchronization, however, since it applies only for inbound connections and considering the topology `node0 <--- node1 <---- node2 <--- ... <-- nodeN`, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.
- https://github.com/bitcoin/bitcoin/pull/29058#issuecomment-1865155764 - "Before enabling -v2transport by default (which I'd image may happen after https://github.com/bitcoin/bitcoin/pull/24748) we could consider a way to force manual connections to be only-v1 or even only-v2 (disabling reconnect-with-v1). A possibility could be through a net permission flag, if https://github.com/bitcoin/bitcoin/pull/27114 makes it in."
- https://github.com/bitcoin/bitcoin/pull/17167#issuecomment-1168606032 - "This would allow us to use https://github.com/bitcoin/bitcoin/pull/25355 when making outgoing connections to all nodes, except to whitelisted ones for which we would use our persistent I2P address."
- Force-relay/mempool permissions for a node you intentionally connected to.
ACKs for top commit:
achow101:
ACK 0a533613fb44207053796fd01a9f4b523a3153d4
sr-gi:
re-ACK [0a53361](https://github.com/bitcoin/bitcoin/pull/27114/commits/0a533613fb44207053796fd01a9f4b523a3153d4)
pinheadmz:
ACK 0a533613fb44207053796fd01a9f4b523a3153d4
Tree-SHA512: 97a79bb854110da04540897d2619eda409d829016aafdf1825ab5515334b0b42ef82f33cd41587af235b3af6ddcec3f2905ca038b5ab22e4c8a03d34f27aebe1
|
