b442580ed2 gui: remove legacy wallet creation (furszy)
Pull request description:
Fixes#763
Preventing users from creating a legacy wallet prior to its deprecation in the upcoming releases.
Note:
This is still available using the `createwallet` RPC command.
Future Note:
Would be nice to re-write this modal as a wizard. And improve the design.
<details><summary> Pre-Changes Screenshot </summary>
<img width="611" alt="Screenshot 2023-10-06 at 11 30 14" src="https://github.com/bitcoin-core/gui/assets/5377650/ca10c97d-46e8-4aed-82da-068f2afbe25c">
</details>
<details><summary> Post-Changes Screenshot </summary>
<img width="729" alt="Screenshot 2023-10-06 at 11 32 58" src="https://github.com/bitcoin-core/gui/assets/5377650/f6bdcb57-646a-43d8-86a7-476e3cca683f">
</details>
ACKs for top commit:
achow101:
ACK b442580ed2
hebasto:
re-ACK b442580ed2
pablomartin4btc:
tACK b442580ed2
Tree-SHA512: f5d26ffbb0962648b9edf273b325e89425a318e136df26a26acb21b88730fd7d6499c68a705680539dc1b40862fbf413a1e0c8572436a0cfc665e2d08a3cf97d
5d227a6862 rpc: Use Ensure(Any)Chainman in assumeutxo related RPCs (Fabian Jahr)
710e5db61b doc: Drop references to assumevalid in assumeutxo docs (Fabian Jahr)
1ff1c34656 test: Rename wait_until_helper to wait_until_helper_internal (Fabian Jahr)
a482f86779 chain: Rename HaveTxsDownloaded to HaveNumChainTxs (Fabian Jahr)
82e48d20f1 blockstorage: Let FlushChainstateBlockFile return true in case of missing cursor (Fabian Jahr)
73700fb554 validation, test: Improve and document nChainTx check for testability (Fabian Jahr)
2c9354facb doc: Add snapshot chainstate removal warning to reindexing documentation (Fabian Jahr)
4e915e926b test: Improvements of feature_assumeutxo (Fabian Jahr)
a47fbe7d49 doc: Add and edit some comments around assumeutxo (Fabian Jahr)
0a39b8cbd8 validation: remove unused mempool param in DetectSnapshotChainstate (Fabian Jahr)
Pull request description:
Addressing what I consider to be non- or not-too-controversial comments from #27596.
Let me know if I missed anything among the many comments that can be easily included here.
ACKs for top commit:
ryanofsky:
Code review ACK 5d227a6862. Just suggested doc change and new EnsureChainman RPC cleanup commit since last review.
Tree-SHA512: 6f7c762100e18f82946b881676db23e67da7dc3a8bf04e4999a183e90b4f150a0b1202bcb95920ba937a358867bbf2eca300bd84b9b1776c7c490410e707c267
- Remove usage of the internal wait_until_helper function
- Use framework self.no_op instead of new no_sync function
co-authored-by: Andrew Chow <github@achow101.com>
0f83ab407e test: display abrupt shutdown errors in console output (furszy)
Pull request description:
Making it easier to debug errors in the CI environment,
particularly in scenarios where it's not immediately clear
what happened nor which node crashed (or shutdown abruptly).
A bit of context:
Currently, the test framework redirects each node's stderr output
stream to a different temporary file inside each node's data directory.
While this is sufficient for storing the error, it isn't very helpful for
understanding what happened just by reading the CI console output.
Most of the time, reading the stderr file in the CI environment is not
possible, because people don't have access to it.
Testing Note:
The displayed error difference can be observed by cherry-picking this
commit 9cc5393c0f on top of this branch and running any
functional test.
ACKs for top commit:
maflcko:
lgtm ACK 0f83ab407e
theStack:
ACK 0f83ab407e
Tree-SHA512: 83ce4d21d5316e8cb16a17d3fbe77b8649fced9e09410861d9674c233f6e9c34bcf573504e387e4f439c2841b2ee9855d0d35607fa13aa89eafe0080c45ee82d
f0cebbdb2a qt: enable -ltcg for windows HOST (fanquake)
Pull request description:
Patch around multiple definition issues in Qt, and enable `-ltcg` when using `LTO=1`.
Split from #25391.
ACKs for top commit:
hebasto:
ACK f0cebbdb2a
Tree-SHA512: 2d6e34779f360bf6dfea4f70fc9004a16e95da79716fcb3046afbf2b01317b7e16965cb51b967b7b5fb64549306c5f48cf59082884289c52016bc1e86949e062
The external input weight test in wallet_fundrawtransaction.py made
transactions at the minimum relay fee. However due to ECDSA sometimes
making a shorter signature than expected, the size estimate (and
therefore the funded fee) ends up being a little bit too low, which
results in the final transaction being under the min relay fee. We can
compensate for this by just using a feerate higher than the minrelayfee
as the actual feerate itself does not matter in this test.
5b878be742 [doc] add release note for submitpackage (glozow)
7a9bb2a2a5 [rpc] allow submitpackage to be called outside of regtest (glozow)
5b9087a9a7 [rpc] require package to be a tree in submitpackage (glozow)
e32ba1599c [txpackages] IsChildWithParentsTree() (glozow)
b4f28cc345 [doc] parent pay for child in aggregate CheckFeeRate (glozow)
Pull request description:
Permit (restricted topology) submitpackage RPC outside of regtest. Suggested in https://github.com/bitcoin/bitcoin/pull/26933#issuecomment-1510851570
This RPC should be safe but still experimental - interface may change, not all features (e.g. package RBF) are implemented, etc. If a miner wants to expose this to people, they can effectively use "package relay" before the p2p changes are implemented. However, please note **this is not package relay**; transactions submitted this way will not relay to other nodes if the feerates are below their mempool min fee. Users should put this behind some kind of rate limit or permissions.
ACKs for top commit:
instagibbs:
ACK 5b878be742
achow101:
ACK 5b878be742
dergoegge:
Code review ACK 5b878be742
ajtowns:
ACK 5b878be742
ariard:
Code Review ACK 5b878be742. Though didn’t manually test the PR.
Tree-SHA512: 610365c0b2ffcccd55dedd1151879c82de1027e3319712bcb11d54f2467afaae4d05dca5f4b25f03354c80845fef538d3938b958174dda8b14c10670537a6524
fa071aeb61 wallet: No BDB creation, unless -deprecatedrpc=create_bdb (MarcoFalke)
Pull request description:
With BDB being removed soon, it seems confusing and harmful to allow users to create fresh BDB wallets going forward, as it would load them with an additional burden of having to migrate them soon after.
Also, it would be good to allow for one release for test (and external) scripts to adapt.
Fix all issues by introducing the `-deprecatedrpc=create_bdb` setting.
ACKs for top commit:
Sjors:
tACK fa071aeb61
achow101:
ACK fa071aeb61
furszy:
utACK fa071aeb
Tree-SHA512: 37a4c3e4ba659e0ebe2382e71d9c80e42a895d9ad743f5dda7c110fbbb7d2a36f46769982552a9ac0c3a57203379ef164be97aa8033eb7674d6b4da030ba8f9b
a9ef702a87 assumeutxo: change getchainstates RPC to return a list of chainstates (Ryan Ofsky)
Pull request description:
Current `getchainstates` RPC returns "normal" and "snapshot" fields which are not ideal because it requires new "normal" and "snapshot" terms to be defined, and the definitions are not really consistent with internal code. (In the RPC interface, the "snapshot" chainstate becomes the "normal" chainstate after it is validated, while in internal code there is no "normal chainstate" and the "snapshot chainstate" is still called that temporarily after it is validated).
The current `getchainstates` RPC is also awkward to use if you to want information about the most-work chainstate, because you have to look at the "snapshot" field if it exists, and otherwise fall back to the "normal" field.
Fix these issues by having `getchainstates` just return a flat list of chainstates ordered by work, and adding a new chainstate "validated" field alongside the existing "snapshot_blockhash" field so it is explicit if a chainstate was originally loaded from a snapshot, and whether the snapshot has been validated.
This change was motivated by comment thread in https://github.com/bitcoin/bitcoin/pull/28562#discussion_r1344154808
ACKs for top commit:
Sjors:
re-ACK a9ef702a87
jamesob:
re-ACK a9ef702
achow101:
ACK a9ef702a87
Tree-SHA512: b364e2e96675fb7beaaee60c4dff4b69e6bc2d8a30dea1ba094265633d1cddf9dbf1c5ce20c07d6e23222cf1e92a195acf6227e4901f3962e81a1e53a43490aa
c1e6c542af descriptors: disallow hybrid public keys (Pieter Wuille)
Pull request description:
Fixes#28511
The descriptor documentation (`doc/descriptors.md`) and [BIP380](https://github.com/bitcoin/bips/blob/master/bip-0380.mediawiki) explicitly require that hex-encoded public keys start with 02 or 03 (compressed) or 04 (uncompressed). However, the current parsing/inference code permit 06 and 07 (hybrid) encoding as well. Fix this.
ACKs for top commit:
darosior:
ACK c1e6c542af
achow101:
ACK c1e6c542af
Tree-SHA512: 23b674fb420619b2536d12da10008bb87cf7bc0333ec59e618c0d02c3574b468cc71248475ece37f76658d743ef51e68566948e903bca79fda5f7d75416fea4d
afd9a673c4 test: roundtrip wallet backwards compat downgrade (Andrew Chow)
bbf43c63b9 test: Add 25.0 to wallet backwards compatibiilty test (Andrew Chow)
538939ec39 test: Run upgrade test on all nodes (Andrew Chow)
6d4699028b test: Run downgrade test on descriptor wallets (Andrew Chow)
f158573be1 test: Add 0.21 tr() incompatibility test (Andrew Chow)
f41215c3f0 test: add logging 0.17 incompatibilities in wallet back compat (Andrew Chow)
71c03aeff7 test: Refactor v19 addmultisigaddress test to be distinct (Andrew Chow)
53f35d02cb test: Remove w1_v18 from wallet backwards compatibility (Andrew Chow)
313d665437 test: Fix 0.16 wallet paths and downgrade test (Andrew Chow)
5d8469362a test: Add helper functions for checking node versions (Andrew Chow)
Pull request description:
It was somewhat surprising to me that wallet_backwards_compatibility.py did not catch #27915 since the purpose of the test is to find downgrade issues such as that. It turns out the test was deficient in several places when it came to testing descriptor wallets, as well as deficient in addition to failing to correctly test some releases.
This PR fixes these test cases, adds more informative logging, slightly refactors the entire test in order to better test future versions, and adds a 25.0 node to the test.
Notable changes:
* The compatibility test with 0.16 should not have been passing. The wallets were being copied incorrectly for 0.16 and resulting in 0.16 creating new wallets rather than testing the target wallets.
* The downgrade test will actually be run on descriptor wallets and it will test that downgrades are successful, and a subsequent upgrade is also successful. This catches #27915.
* The upgrade and downgrade test will be run on all versions up to master, rather than just 0.16, 0.17, and 0.19.
ACKs for top commit:
Sjors:
re-ACK afd9a673c4
furszy:
ACK afd9a67
Tree-SHA512: dd2d85cab29a636da93020681c533534af4a9cda18d8550c9db9d8937719b3a225025966981c5d4d2f30486448a772b760f0e723a25ea6bc49df80387dc7b8b0
fa28f5a381 test: Bump walletpassphrase timeouts to avoid intermittent issues (MarcoFalke)
Pull request description:
This bumps all timeouts for all `walletpassphrase` to avoid intermittent issues in `valgrind` (or other sanitizers).
As an idea for a follow-up, `walletpassphrase` could be changed to treat `0` as "no timeout" instead of "instant timeout".
Example failure:
```
node0 2023-09-03T22:44:38.374955Z [httpworker.3] [rpc/server.cpp:594] [RPCRunLater] [rpc] queue run of timer lockwallet(w6) in 60 seconds (using HTTP)
test 2023-09-03T22:44:40.173000Z TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-rpcwallet=w6', 'getnewaddress', '', 'legacy']
node0 2023-09-03T22:44:59.810893Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for /wallet/w6 from 127.0.0.1:48928
node0 2023-09-03T22:44:59.813132Z [httpworker.1] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=getnewaddress user=__cookie__
node0 2023-09-03T22:44:59.837183Z [httpworker.1] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20230903_183350/wallet_createwallet_171/node0/regtest/w6/wallet.dat] SQLite Statement: INSERT or REPLACE into main values(?, ?)
node0 2023-09-03T22:44:59.929735Z [httpworker.1] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20230903_183350/wallet_createwallet_171/node0/regtest/w6/wallet.dat] SQLite Statement: INSERT or REPLACE into main values(?, ?)
node0 2023-09-03T22:44:59.934484Z [httpworker.1] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20230903_183350/wallet_createwallet_171/node0/regtest/w6/wallet.dat] SQLite Statement: INSERT or REPLACE into main values(?, ?)
node0 2023-09-03T22:44:59.935467Z [httpworker.1] [wallet/sqlite.cpp:53] [TraceSqlCallback] [/ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20230903_183350/wallet_createwallet_171/node0/regtest/w6/wallet.dat] SQLite Statement: INSERT or REPLACE into main values(?, ?)
test 2023-09-03T22:45:02.328000Z TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-rpcwallet=w6', 'signmessage', 'mqatqH4VQmrZ81nxUfrnfcLnxgbzhZb4PC', 'test']
node0 2023-09-03T22:45:20.269375Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for /wallet/w6 from 127.0.0.1:44618
node0 2023-09-03T22:45:20.270670Z [httpworker.2] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=signmessage user=__cookie__
test 2023-09-03T22:45:23.490000Z TestFramework.bitcoincli (DEBUG): Running bitcoin-cli ['-rpcwallet=w6', 'keypoolrefill', '1']
node0 2023-09-03T22:45:40.244603Z [http] [httpserver.cpp:255] [http_request_cb] [http] Received a POST request for /wallet/w6 from 127.0.0.1:32854
node0 2023-09-03T22:45:40.293021Z [httpworker.0] [rpc/request.cpp:181] [parse] [rpc] ThreadRPCServer method=keypoolrefill user=__cookie__
test 2023-09-03T22:45:41.852000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
self.run_test()
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_createwallet.py", line 156, in run_test
w6.keypoolrefill(1)
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 732, in __call__
return self.cli.send_cli(self.command, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/ci_container_base/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_node.py", line 795, in send_cli
raise JSONRPCException(dict(code=int(code), message=message))
test_framework.authproxy.JSONRPCException: Error: Please enter the wallet passphrase with walletpassphrase first. (-13)
ACKs for top commit:
achow101:
ACK fa28f5a381
Tree-SHA512: 58caa569cec39acc121d4cc038a4190937af34e85d2696272ed4f2792fd386469b0cfefd2cb564438fedded97b21b23d8bf46ba27b5633671a277ed4679f0d5d
Instead of figuring out the commit *after* the last merge and rebasing on that
with a ~1 suffix, just figure out the last merge commit directly and rebase on
it. This way, if HEAD happens to be a merge commit, the rebase just succeeds
immediately without blank variables or errors.
From https://github.com/bitcoin/bitcoin/pull/28497#issuecomment-1743430631:
The problem is that the PR only contains a one commit after the last merge,
so the job _should_ be skipped, but the `pull_request.commits != 1` check
is not smart enough to skip it because the PR is based on another PR and
has merge ancestor commits. So specifically what happens is that after
HEAD~ is checked out, the new HEAD is a merge commit, so the range `$(git
log --merges -1 --format=%H)..HEAD` is equivalent to HEAD..HEAD, which is
empty, so the `COMMIT_AFTER_LAST_MERGE` variable is empty and the rebase
command fails.
Current getchainstates RPC returns "normal" and "snapshot" fields which are not
ideal because it requires new "normal" and "snapshot" terms to be defined, and
the definitions are not really consistent with internal code. (In the RPC
interface, the "snapshot" chainstate becomes the "normal" chainstate after it
is validated, while in internal code there is no "normal chainstate" and the
"snapshot chainstate" is still called that temporarily after it is validated).
The current getchainstatees RPC is also awkward to use if you to want
information about the most-work chainstate because you have to look at the
"snapshot" field if it exists, and otherwise fall back to the "normal" field.
Fix these issues by having getchainstates just return a flat list of
chainstates ordered by work, and adding new chainstate "validated" field
alongside the existing "snapshot_blockhash" so it is explicit if a chainstate
was originally loaded from a snapshot, and whether the snapshot has been
validated.
`vfLimited`, `IsReachable()`, `SetReachable()` need not be in the `net`
module. Move them to `netbase` because they will be needed in
`LookupSubNet()` to possibly flip the result to CJDNS (if that network
is reachable).
In the process, encapsulate them in a class.
`NET_UNROUTABLE` and `NET_INTERNAL` are no longer ignored when adding
or removing reachable networks. This was unnecessary.
Making it easier to debug errors in the CI environment,
particularly in scenarios where it's not immediately clear
what happened nor which node crashed (or shutdown abruptly).
From https://geti2p.net/en/docs/api/samv3:
If SILENT=false was passed, which is the default value, the SAM bridge
sends the client a ASCII line containing the base64 public destination
key of the requesting peer
So, `Accept()` is supposed to receive a Base64 encoded destination of
the connecting peer, but if it receives something like this instead:
STREAM STATUS RESULT=I2P_ERROR MESSAGE="Session was closed"
then destroy the session.
Background:
`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`)
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
vanished long time ago.
* Increment the wait time less aggressively.
e1308967e1 test: BIP324: add checks for v1 prefix matching / wrong network magic detection (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the detection of incoming v1 connections and wrong network magic on BIP324-enabled (i.e. running with `-v2transport=1`) nodes. Both checks are using prefix sizes of 16 bytes (previously only 12 bytes were used for the v1 prefix matching, which was fixed by PR #28577).
ACKs for top commit:
Sjors:
utACK e1308967e1
MarcoFalke:
lgtm ACK e1308967e1
Tree-SHA512: d4d1567277297f42c543b9638a6c64d14b17ff0ddbf85a7efff22f45c619478139dbedcb9dc4f449b4814b00856ee43644f15df1aa20c8931d5496a607ca2fd4
3d420d8f28 Add instructions for headerssync-params.py to release-process.md (Pieter Wuille)
53d7d35b58 Update parameters in headerssync.cpp (Pieter Wuille)
7899402cff Add headerssync-params.py script to the repository (Pieter Wuille)
Pull request description:
Builds upon #25946, as it incorporates changes based on the selected values there.
This adds the headerssync tuning parameters optimization script from https://gist.github.com/sipa/016ae445c132cdf65a2791534dfb7ae1 to the repository, updates the parameters based on its output, and adds release process instructions for doing this update in the future.
A few considerations:
* It would be a bit cleaner to have these parameters be part of `CChainParams`, but due to the nature of the approach, it really only applies to chains with unforgeable proof-of-work, which we really can only reasonably expect from mainnet, so I think it's fine to keep them local to `headerssync.cpp`. Keeping them as compile-time evaluatable constants also has a (likely negligible) performance impact (avoiding runtime modulo operations).
* If we want to make sure the chainparams and headerssync params don't go out of date, it could be possible to run the script in CI, and and possibly even have the parameters be generated automatically at build time. I think that's overkill for how unfrequently these need to change, and running the script has non-trivial cost (~minutes in the normal python interpreter).
* A viable alternative is just leaving this out-of-repo entirely, and just do ad-hoc updating from time to time. Having it in the repo and release notes does make sure it's not forgotten, though adds a cost to contributors/maintainers who follow the process.
ACKs for top commit:
ajtowns:
reACK 3d420d8f28
Tree-SHA512: 03188301c20423c72c1cbd008ccce89b93e2898edcbeecc561b2928a0d64e9a829ab0744dc3b017c23de8b02f3c107ae31e694302d3931f4dc3540e184de1963
ba2e5bfc67 net: raise V1_PREFIX_LEN from 12 to 16 (Pieter Wuille)
Pull request description:
A "version" message in the V1 protocol starts with a fixed 16 bytes:
* The 4-byte network magic
* The 12-byte command string: "version" plus 5 0x00 bytes
The current code detects incoming V1 connections by just looking at the first 12 bytes (matching an [earlier version](https://github.com/bitcoin/bips/pull/1496) of BIP324), but 16 bytes is more precise. This isn't an observable difference right now, as a 12 byte prefix ought to be negligible already, but it may become observable with future extensions to the protocol, so make the code match the specification.
ACKs for top commit:
achow101:
ACK ba2e5bfc67
theStack:
re-ACK ba2e5bfc67
mzumsande:
Code review ACK ba2e5bfc67
Tree-SHA512: 64876b03613bd1c5dda82f4ca1b367014365f9ae4cfa30f45c5758a563c68cbea81a98d02ba616c264674c204517aac8b7de94da10f32e77b56267a43710c651
58c9b50a95 gui: Add wallet name to address book page (pablomartin4btc)
Pull request description:
It fixesbitcoin-core/gui#756.
Each address book page window it's now labeled with the wallet name they were opened with, so the user can easily identify which addresses belong to which wallet even when there are many windows opened. It's a helpful enhancement for users managing multiple wallets.
ACKs for top commit:
hebasto:
ACK 58c9b50a95, tested on Ubuntu 22.04.
Tree-SHA512: 82febc020653560281da144cd35c672c49ca9f48b23d173eb19395e9ab4d045500295a9b5f24c82efdbf6e7ea70c87e733207cb6a31d3f84828b27e3b2df558b
