| Age | Commit message (Collapse) | Author |
|---|
|
|
|
after migration
4814e4063e674ad9b0a5c7e56059cd6a2bf9b764 test: Check tx metadata is migrated to watchonly (Andrew Chow)
d616d30ea5fdfb897f8375ffd8b9f4536ae7835b wallet: Reload watchonly and solvables wallets after migration (Andrew Chow)
118f2d7d70b584eee7b89e58b5cd2d61c59a9bbf wallet: Copy all tx metadata to watchonly wallet (Andrew Chow)
9af87cf3485ce3fac553a284cde37a35d1085c25 test: Check that a failed wallet migration is cleaned up (Andrew Chow)
Pull request description:
Some incomplete/incorrect state as a result of migration can be mitigated/cleaned up by simply restarting the migrated wallets. We already do this for a wallet when it is migrated, but we do not for the new watchonly and solvables wallets that may be created. This PR introduces this behavior, in addition to creating those wallets initially without an attached chain.
While implementing this, I noticed that not all `CWalletTx` metadata was being copied over to the watchonly wallet and so some data, such as time received, was being lost. This PR fixes this as a side effect of not having a chain attached to the watchonly wallet. A test has also been added.
ACKs for top commit:
ishaanam:
light code review ACK 4814e4063e674ad9b0a5c7e56059cd6a2bf9b764
ryanofsky:
Code review ACK 4814e4063e674ad9b0a5c7e56059cd6a2bf9b764. Just implemented the suggested orderpos, copyfrom, and path set comments since last review
furszy:
ACK 4814e406
Tree-SHA512: 0b992430df9f452cb252c2212df8e876613f43564fcd1dc00c6c31fa497adb84dfff6b5ef597590f9b288c5f64cb455f108fcc9b6c9d1fe9eb2c39e7f2c12a89
|
|
calculation
4bfaad4eca01674a9c84a447a17594dc2b9a4c39 chainparams, assumeutxo: Fix signet txoutset hash (Fabian Jahr)
a503cd0f0b55736743bcf8d2c46d271064772bef chainparams, assumeutxo: Fix testnet txoutset hash (Fabian Jahr)
f6213929c519d0e615cacd3d6f479f1517be1662 assumeutxo: Check deserialized coins for out of range values (Fabian Jahr)
66865446a771327be9e972cdaf01154ea1bdff6d docs: Add release notes for #28685 (Fabian Jahr)
cb0336817edc2b6aee2eca818f133841f613a767 scripted-diff: Rename hash_serialized_2 to hash_serialized_3 (Fabian Jahr)
351370a1d211615e3d5b158ccb0400cd79c5c085 coinstats: Fix hash_serialized2 calculation (Fabian Jahr)
Pull request description:
Closes #28675
The last commit demonstrates that theStack's analysis [here](https://github.com/bitcoin/bitcoin/issues/28675#issuecomment-1770389468) seems to be correct. There will be more changes needed for the rest of the test suite but the `feature_assumeutxo.py` with my additional tests pass.
ACKs for top commit:
achow101:
ACK 4bfaad4eca01674a9c84a447a17594dc2b9a4c39
theStack:
Code-review ACK 4bfaad4eca01674a9c84a447a17594dc2b9a4c39
ryanofsky:
Code review ACK 4bfaad4eca01674a9c84a447a17594dc2b9a4c39
Tree-SHA512: 2f6abc92b282f7c5da46391803cf0804d13978d191d541f2509b532c538abccd0a081e46cda23d80d47206a05fa2b5d41b7ab246e6a263db7a7461d6292116ef
|
|
This reverts commit 7066e8996d0ac090535cc97cdcb54a219986460f.
|
|
explicitly in `util/trace.h`
6bdff429ec17eae4138c3af1e21de3ec46f4ab13 build: Include `config/bitcoin-config.h` explicitly in `util/trace.h` (Hennadii Stepanov)
Pull request description:
The `ENABLE_TRACING` macro is expected to be defined in the `config/bitcoin-config.h` header.
Therefore, the current code is error-prone as it depends on whether the `config/bitcoin-config.h` header was included before or not.
This bug was noticed while working on CMake [stuff](https://github.com/hebasto/bitcoin/pull/37).
ACKs for top commit:
fanquake:
ACK 6bdff429ec17eae4138c3af1e21de3ec46f4ab13
Tree-SHA512: 22c4fdeb51628814050eb99a83db4268a4f3106207eeef918a07214bbc52f2b22490f6b05fcb96216f147afa4197c51102503738131e2583e750b6d195747a49
|
|
fac36b94ef32567c0f10b605a3a441d11559e56e refactor: Remove CBlockFileInfo::SetNull (MarcoFalke)
Pull request description:
Seems better to use C++11 member initializers and then let the compiler figure out how to construct objects of this class.
ACKs for top commit:
stickies-v:
ACK fac36b94ef32567c0f10b605a3a441d11559e56e
pablomartin4btc:
ACK fac36b94ef32567c0f10b605a3a441d11559e56e
theStack:
LGTM ACK fac36b94ef32567c0f10b605a3a441d11559e56e
Tree-SHA512: aee741c8f668f0e5b658fc83f4ebd196b43fead3dd437afdb0a2dafe092ae3d559332b3d9d61985c92e1a59982d8f24942606e6a98598c6ef7ff43697e858725
|
|
Review hint: You can use devtools/utxo_snapshot.sh to validate this.
./contrib/devtools/utxo_snapshot.sh 160000 signet-utxo.dat ./src/bitcoin-cli
|
|
Review hint: You can use devtools/utxo_snapshot.sh to validate this.
./contrib/devtools/utxo_snapshot.sh 2500000 testnet-utxo.dat ./src/bitcoin-cli
|
|
|
|
-BEGIN VERIFY SCRIPT-
sed -i 's/hash_serialized_2/hash_serialized_3/g' $( git grep -l 'hash_serialized_2' ./src ./contrib ./test )
-END VERIFY SCRIPT-
|
|
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>
|
|
dd4dcbd4cda31f67d014a93340a6d1ba1c245b0f [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 dd4dcbd4cda31f67d014a93340a6d1ba1c245b0f
glozow:
ACK dd4dcbd4cda31f67d014a93340a6d1ba1c245b0f, agree it's better to delete this test until somebody wants to write a better one
Tree-SHA512: b6ca6cad1773b1ceb6e5ac0fd501ea615f66507ef811745799deaaa4460f1700d96ae03cf55b740a96ed8cd2283b3d6738cd580ba97f2af619197d6c4414ca21
|
|
|
|
The `ENABLE_TRACING` macro is expected to be defined in the
`config/bitcoin-config.h` header.
Therefore, the current code is error-prone as it depends on whether the
`config/bitcoin-config.h` header was included before or not.
|
|
a snapshot chainstate
ec84f999f1408b7f1ff4498f78c33b34c30e934c log: Don't log cache rebalancing in absense of a snapshot chainstate (Fabian Jahr)
Pull request description:
I have noticed that this log now is always printed, even if there is no snapshot chainstate present or even was present. I think this is confusing to users that have never even thought about using assumeutxo since in that case the rebalancing is just ensuring the normal environment with one chainstate. So I suggest we don't log in absence of a snapshot chainstate. We could also think about rewording the message instead but I think this is simpler.
ACKs for top commit:
stickies-v:
utACK ec84f999f1408b7f1ff4498f78c33b34c30e934c
glozow:
concept ACK ec84f999f1408b7f1ff4498f78c33b34c30e934c, don't have opinions other than removing confusing log
theStack:
utACK ec84f999f1408b7f1ff4498f78c33b34c30e934c
Tree-SHA512: 30bbfc648e7c788106f78d52e47a3aa1e1874f65d13743643dc50bcf7f450d8330711ff9fdeac361722542da6051533153829c6d49033227ed315e111afc899f
|
|
|
|
|
|
When migrating, create the watchonly and solvables wallets without a
context. Then unload and reload them after migration completes, as we do
for the actual wallet.
There is also additional handling for a failed reload.
|
|
When moving a tx to the watchonly wallet during migration, make sure
that all of the CWalletTx data follows it.
|
|
destroy the session if we get an unexpected error
5c8e15c451ec870b9dd4eb843ec6ca3ba64cda4f i2p: destroy the session if we get an unexpected error from the I2P router (Vasil Dimov)
762404a68c114e8831cdfae937627174544b55a7 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 5c8e15c451ec870b9dd4eb843ec6ca3ba64cda4f
jonatack:
re-ACK 5c8e15c451ec870b9dd4eb843ec6ca3ba64cda4f
Tree-SHA512: 1d47958c50eeae9eefcb668b8539fd092adead93328e4bf3355267819304b99ab41cbe1b5dbedbc3452c2bc389dc8330c0e27eb5ccb880e33dc46930a1592885
|
|
0e6f6ebc064c5fb425fc3699efe760ec6cd4b6af net: remove unused CConnman::FindNode(const CSubNet&) (Vasil Dimov)
9482cb780fe04c1f1d9050edd1b8e549e52c86ce netbase: possibly change the result of LookupSubNet() to CJDNS (Vasil Dimov)
53afa68026ffa1313ae4aba3664de7791d23b1c8 net: move MaybeFlipIPv6toCJDNS() from net to netbase (Vasil Dimov)
6e308651c441cbf8763c67cc099c538c333c2872 net: move IsReachable() code to netbase and encapsulate it (Vasil Dimov)
c42ded3d9bda8b273780a4a81490bbf1b9e9c261 fuzz: ConsumeNetAddr(): avoid IPv6 addresses that look like CJDNS (Vasil Dimov)
64d6f77907afd461d9b14ee10ab32335f4454734 net: put CJDNS prefix byte in a constant (Vasil Dimov)
Pull request description:
`LookupSubNet()` would treat addresses that start with `fc` as IPv6 even if `-cjdnsreachable` is set. This creates the following problems where it is called:
* `NetWhitelistPermissions::TryParse()`: otherwise `-whitelist=` fails to white list CJDNS addresses: when a CJDNS peer connects to us, it will be matched against IPv6 `fc...` subnet and the match will never succeed.
* `BanMapFromJson()`: CJDNS bans are stored as just IPv6 addresses in `banlist.json`. Upon reading from disk they have to be converted back to CJDNS, otherwise, after restart, a ban entry like (`fc00::1`, IPv6) would not match a peer (`fc00::1`, CJDNS).
* `RPCConsole::unbanSelectedNode()`: in the GUI the ban entries go through `CSubNet::ToString()` and back via `LookupSubNet()`. Then it must match whatever is stored in `BanMan`, otherwise it is impossible to unban via the GUI.
These were uncovered by https://github.com/bitcoin/bitcoin/pull/26859.
Thus, flip the result of `LookupSubNet()` to CJDNS if the network base address starts with `fc` and `-cjdnsreachable` is set. Since subnetting/masking does not make sense for CJDNS (the address is "random" bytes, like Tor and I2P, there is no hierarchy) treat `fc.../mask` as an invalid `CSubNet`.
To achieve that, `MaybeFlipIPv6toCJDNS()` has to be moved from `net` to `netbase` and thus also `IsReachable()`. In the process of moving `IsReachable()`, `SetReachable()` and `vfLimited[]` encapsulate those in a class.
ACKs for top commit:
jonatack:
Code review ACK 0e6f6ebc064c5fb425fc3699efe760ec6cd4b6af
achow101:
ACK 0e6f6ebc064c5fb425fc3699efe760ec6cd4b6af
mzumsande:
re-ACK 0e6f6ebc064c5fb425fc3699efe760ec6cd4b6af
Tree-SHA512: 4767a60dc882916de4c8b110ce8de208ff3f58daaa0b560e6547d72e604d07c4157e72cf98b237228310fc05c0a3922f446674492e2ba02e990a272d288bd566
|
|
extract_strings.py
b59b31ae0b04054c5cf225dad87046d3771707fc build: Drop redundant qt/bitcoin.cpp (Hennadii Stepanov)
d90ad5a42ec6f48d0e504edc16d41c8ef266cc1d build: Include qt sources for parsing with extract_strings.py (Hennadii Stepanov)
Pull request description:
On master (4fc15d15667d9d9c4fb5515ce73c05b4596298ec) some strings are still untranslated.
This PR fixes this issue.
To verify:
1) `./autogen.sh && ./configure && make -C src translate` _before_ applying this change
2) apply this change
3) `./autogen.sh && ./configure && make -C src translate` _after_ applying this change
The result of `git diff src/qt/bitcoinstrings.cpp`:
```diff
--- a/src/qt/bitcoinstrings.cpp
+++ b/src/qt/bitcoinstrings.cpp
@@ -126,6 +126,7 @@ QT_TRANSLATE_NOOP("bitcoin-core", ""
"You need to rebuild the database using -reindex to go back to unpruned "
"mode. This will redownload the entire blockchain"),
QT_TRANSLATE_NOOP("bitcoin-core", "%s is set very high!"),
+QT_TRANSLATE_NOOP("bitcoin-core", "(press q to shutdown and continue later)"),
QT_TRANSLATE_NOOP("bitcoin-core", "-maxmempool must be at least %d MB"),
QT_TRANSLATE_NOOP("bitcoin-core", "A fatal internal error occurred, see debug.log for details"),
QT_TRANSLATE_NOOP("bitcoin-core", "Cannot resolve -%s address: '%s'"),
@@ -204,6 +205,8 @@ QT_TRANSLATE_NOOP("bitcoin-core", "SQLiteDatabase: Failed to prepare statement t
QT_TRANSLATE_NOOP("bitcoin-core", "SQLiteDatabase: Failed to read database verification error: %s"),
QT_TRANSLATE_NOOP("bitcoin-core", "SQLiteDatabase: Unexpected application id. Expected %u, got %u"),
QT_TRANSLATE_NOOP("bitcoin-core", "Section [%s] is not recognized."),
+QT_TRANSLATE_NOOP("bitcoin-core", "Settings file could not be read"),
+QT_TRANSLATE_NOOP("bitcoin-core", "Settings file could not be written"),
QT_TRANSLATE_NOOP("bitcoin-core", "Signing transaction failed"),
QT_TRANSLATE_NOOP("bitcoin-core", "Specified -walletdir \"%s\" does not exist"),
QT_TRANSLATE_NOOP("bitcoin-core", "Specified -walletdir \"%s\" is a relative path"),
@@ -242,4 +245,5 @@ QT_TRANSLATE_NOOP("bitcoin-core", "User Agent comment (%s) contains unsafe chara
QT_TRANSLATE_NOOP("bitcoin-core", "Verifying blocks…"),
QT_TRANSLATE_NOOP("bitcoin-core", "Verifying wallet(s)…"),
QT_TRANSLATE_NOOP("bitcoin-core", "Wallet needed to be rewritten: restart %s to complete"),
+QT_TRANSLATE_NOOP("bitcoin-core", "press q to shutdown"),
};
```
ACKs for top commit:
ryanofsky:
Code review ACK b59b31ae0b04054c5cf225dad87046d3771707fc. Being able to use `_()` macro in qt would allow simplifying some code, for example replacing repetitive:
TheCharlatan:
ACK b59b31ae0b04054c5cf225dad87046d3771707fc
Tree-SHA512: 13d9d86b487a1b6e718ae96c198a0a927c881bf33df318412793ec9efba3a7e59cfa836204f73f5b53ff4c99edce778c11bffaa88138b80e37b71e36df6b816f
|
|
tapscript
b22810887b3840ad0fcb424ea7e16d2d195767d9 miniscript: make GetWitnessSize accurate for tapscript (Pieter Wuille)
8be98514080ab816fcb2498ea4bc6f211a2b05e0 test: add tests for miniscript GetWitnessSize (Pieter Wuille)
7ed2b2d430e4dc0d3ba62a30f814df2c7c0c0651 test: remove mutable global contexts in miniscript fuzzer/test (Pieter Wuille)
Pull request description:
So far, the same algorithm is used to compute an (upper bound on) the maximum witness size for both P2WSH and P2TR miniscript. That's unfortunate, because it means fee estimations for P2TR miniscript will miss out on the generic savings brought by P2TR witnesses (smaller signatures and public keys, specifically).
Fix this by making the algorithm use script context specification calculations, and add tests for it. Also included is a cleanup for the tests to avoid mutable globals, as I found it hard to reason about what exactly was being tested.
ACKs for top commit:
achow101:
ACK b22810887b3840ad0fcb424ea7e16d2d195767d9
darosior:
ACK b22810887b3840ad0fcb424ea7e16d2d195767d9
Tree-SHA512: e4bda7376628f3e91cfc74917cefc554ca16eb5f2a0e1adddc33eb8717c4aaa071e56a40f85a2041ae74ec445a7bd0129bba48994c203e0e6e4d25af65954d9e
|
|
taproot spends
00a52e63946d5a90cdfb68204373d9c23d885161 gui: fix coin control input size accounting for taproot spends (Sebastian Falbesoner)
Pull request description:
If manual coin control is used in the GUI, the input size accounting for P2TR is currently overshooting, as it still assumes P2WPKH (segwitv0) spends which have a larger witness, as ECDSA signatures are longer and the pubkey also has to be provided. Fix that by adding sizes depending on the witness version. Note that the total accounting including outputs is still off and there is some weird logic involved depending on whether SFFO is used, but it's (hopefully) a first step into the right direction.
ACKs for top commit:
maflcko:
lgtm ACK 00a52e63946d5a90cdfb68204373d9c23d885161
furszy:
utACK 00a52e6394
Tree-SHA512: 9633642f8473247cc3d8e6e0ef502fd515e1dde0e2939d28d6754d0cececedd6a328df22a3d4c85eb2846fd0417cf224b92594613f6e84ada82d2d7d84fc455f
|
|
debugging and logging
8a553c94098c96cb3679468c2b460be145a0eabf wallet: Add TxStateString function for debugging and logging (Ryan Ofsky)
Pull request description:
I found this useful while debugging silent conflict between #10102 and #27469 recently
ACKs for top commit:
ishaanam:
utACK 8a553c94098c96cb3679468c2b460be145a0eabf
achow101:
ACK 8a553c94098c96cb3679468c2b460be145a0eabf
furszy:
Code ACK 8a553c9
Tree-SHA512: 87965c66bcb59a21e7639878bb567e583a0e624735721ff7ad1104eed6bb9fba60607d0e3de7be3304232b3a55f48bab7039ea9c26b0e81963e59f9acd94f666
|
|
doesn't match AssumeUTXO parameters
9620cb449374f234f72c1a9e1bae3d4b8c0ff171 assumeutxo: fail early if snapshot block hash doesn't match AssumeUTXO parameters (Sebastian Falbesoner)
Pull request description:
Right now the `loadtxoutset` RPC call treats literally all files with a minimum size of 40 bytes (=size of metadata) as potential valid snapshot candidates and the waiting loop for seeing the metadata block hash in the headers chain is always entered, e.g.:
```
$ ./src/bitcoin-cli loadtxoutset ~/.vimrc
<wait>
bitcoind log:
...
2023-10-15T14:55:45Z [snapshot] waiting to see blockheader 626174207465730a7265626d756e207465730a656c62616e65207861746e7973 in headers chain before snapshot activation
...
```
There is no point in doing any further action though if we already know from the start that the UTXO snapshot loading won't be successful. This PR adds an assumeutxo parameter check immediately after the metadata is read in, so we can fail immediately on a mismatch:
```
$ ./src/bitcoin-cli loadtxoutset ~/.vimrc
error code: -32603
error message:
Unable to load UTXO snapshot, assumeutxo block hash in snapshot metadata not recognized (626174207465730a7265626d756e207465730a656c62616e
65207861746e7973)
```
This way, users who mistakenly try to load files that are not snapshots don't have to wait 10 minutes (=the block header waiting timeout) anymore to get a negative response. If a file is loaded which is a valid snapshot (referencing to an existing block hash), but one which doesn't match the parameters, the feedback is also faster, as we don't have to wait anymore to see the hash in the headers chain before getting an error.
This is also partially fixes #28621.
ACKs for top commit:
maflcko:
lgtm ACK 9620cb449374f234f72c1a9e1bae3d4b8c0ff171
ryanofsky:
Code review ACK 9620cb449374f234f72c1a9e1bae3d4b8c0ff171. This should fix an annoyance and bad UX.
pablomartin4btc:
tACK 9620cb449374f234f72c1a9e1bae3d4b8c0ff171
Tree-SHA512: f88b865e9d46254858e57c024463f389cd9d8760a7cb30c190aa1723a931e159987dfc2263a733825d700fa612e7416691e4d8aab64058f1aeb0a7fa9233ac9c
|
|
ff8e2fc2e2416f6f3b84cdb40db8ac168596b579 fuzz: add coverage for `bitcoinconsensus_verify_script_with_spent_outputs` (brunoerg)
c5f2a757d736f14d27ac5256a9df887cd2f174f1 docs: add release notes for #28539 (brunoerg)
de54882348502d860cf1e504100aa8fb1e52aa88 docs: add docs for additional libconsensus functions (Jake Rawsthorne)
70106e0689546fee497814c63a6a4747e0937b36 docs: link to rust-bitcoinconsensus (Jake Rawsthorne)
fb0db07e414fec3318b3af683167ebef9c82fc84 lib: add Taproot support to libconsensus (Jake Rawsthorne)
Pull request description:
Grabbed from #21158. Closes #21133.
ACKs for top commit:
achow101:
ACK ff8e2fc2e2416f6f3b84cdb40db8ac168596b579
theStack:
ACK ff8e2fc2e2416f6f3b84cdb40db8ac168596b579
darosior:
re-ACK ff8e2fc2e2416f6f3b84cdb40db8ac168596b579
Tree-SHA512: bf6f500c7e8c9ff6884137c2cd9b4522c586e52848dd639b774b94d998b0516b877498d24f3a6cc7425aedf81d18b0d30c1ccf19e2d527fdfdfa3955ca49b6e7
|
|
parameters
|
|
fa05a726c225dc65dee79367bb67f099ae4f99e6 tidy: modernize-use-emplace (MarcoFalke)
Pull request description:
Constructing a temporary unnamed object only to copy or move it into a container seems both verbose in code and a strict performance penalty.
Fix both issues via the `modernize-use-emplace` tidy check.
ACKs for top commit:
Sjors:
re-utACK fa05a726c2
hebasto:
ACK fa05a726c225dc65dee79367bb67f099ae4f99e6.
TheCharlatan:
ACK fa05a726c225dc65dee79367bb67f099ae4f99e6
Tree-SHA512: 4408a094f406e7bf6c1468c2b0798f68f4d952a1253cf5b20bdc648ad7eea4a2c070051fed46d66fd37bce2ce6f85962484a1d32826b7ab8c9baba431eaa2765
|
|
8b6470a90652fcffc45b8d7998af7c8ad6251332 gui: disable top bar menu actions during shutdown (furszy)
7066e8996d0ac090535cc97cdcb54a219986460f gui: provide wallet controller context to wallet actions (furszy)
Pull request description:
Small follow-up to #751.
Fixes another crash cause during shutdown. Which occurs when the user hovers over the wallets list.
Future Note:
This surely happen in other places as well, we should re-work the way we connect signals. Register
lambas without any precaution can leave dangling pointers.
ACKs for top commit:
hebasto:
ACK 8b6470a90652fcffc45b8d7998af7c8ad6251332, I've tested each commit separately on macOS Sonoma 14.0 (Apple M1).
Tree-SHA512: 6fbd1bcd6717a8c1633beb9371463ed22422f929cccf9b791ee292c5364134c501e099329cf77a06b74a84c64c1c3d22539199ec49ccd74b3950036316c0dab3
|
|
|
|
All callers of `LookupSubNet()` need the result to be of CJDNS type if
`-cjdnsreachable` is set and the address begins with `fc`:
* `NetWhitelistPermissions::TryParse()`: otherwise `-whitelist=` fails
to white list CJDNS addresses: when a CJDNS peer connects to us, it
will be matched against IPv6 `fc...` subnet and the match will never
succeed.
* `BanMapFromJson()`: CJDNS bans are stored as just IPv6 addresses in
`banlist.json`. Upon reading from disk they have to be converted back
to CJDNS, otherwise, after restart, a ban entry like (`fc00::1`, IPv6)
would not match a peer (`fc00::1`, CJDNS).
* `setban()` (in `rpc/net.cpp`): otherwise `setban fc.../mask add` would
add an IPv6 entry to BanMan. Subnetting does not make sense for CJDNS
addresses, thus treat `fc.../mask` as invalid `CSubNet`. The result of
`LookupHost()` has to be converted for the case of banning a single
host.
* `InitHTTPAllowList()`: not necessary since before this change
`-rpcallowip=fc...` would match IPv6 subnets against IPv6 peers even
if they started with `fc`. But because it is necessary for the above,
`HTTPRequest::GetPeer()` also has to be adjusted to return CJDNS peer,
so that now CJDNS peers are matched against CJDNS subnets.
|
|
e6e444c06cbf09380f9924dff3d21c1be15d1753 refactor: add and use EnsureAnyAddrman in rpc (stratospher)
bf589a50a0d6a7b94f1ba1ddf24a1497fd35ad44 doc: add release notes for #27511 (stratospher)
3931e6abc39b8aee1472028dbf76eeb10708d2b4 rpc: `getaddrmaninfo` followups (stratospher)
Pull request description:
- make `getaddrmaninfo` RPC public since it's not for development purposes only and regular users might find it useful. [#26988 (comment)](https://github.com/bitcoin/bitcoin/pull/26988#issuecomment-1738371584)
- add missing `all_networks` key to RPC help. [#27511 (comment)](https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1335084087)
- fix clang format spacing
- add and use `EnsureAddrman` in RPC code. [#27511 (comment)](https://github.com/bitcoin/bitcoin/pull/27511#discussion_r1331501491)
ACKs for top commit:
0xB10C:
Code Review re-ACK e6e444c06cbf09380f9924dff3d21c1be15d1753
theStack:
Code-review ACK e6e444c06cbf09380f9924dff3d21c1be15d1753
pablomartin4btc:
tested ACK e6e444c06cbf09380f9924dff3d21c1be15d1753
Tree-SHA512: c14090d5c64ff15e92d252578de2437bb2ce2e1e431d6698580241a29190f0a3528ae5b013c0ddb76a9ae538507191295c37cab7fd93469941cadbde44587072
|
|
Opening the top bar menu when the app is being destroyed
freezes the GUI shutdown process for no reason. No menu
action can be executed.
Note:
This behavior is consistent with how the tray icon menu
is cleared too.
|
|
Addressing potential crashes during shutdown. The most
noticeable one can be triggered by hovering over the
wallet list as the app shuts down.
|
|
|
|
|
|
|
|
Co-authored-by: Antonie Poinsot <darosior@protonmail.com>
|
|
Co-authored-by: Bruno Garcia <brunoely.gc@gmail.com>
|
|
SnapshotMetadata constructor, fix test, add test
fafde92f84fb7c245bc3c1cd946a32c891861e5e test: Check snapshot file with wrong number of coins (MarcoFalke)
faa90f6e7b6b8c531e1ae142a5c2f568b48502a9 refactor: Remove unused nchaintx from SnapshotMetadata constructor (MarcoFalke)
Pull request description:
See commit messages
ACKs for top commit:
Sjors:
utACK fafde92f84fb7c245bc3c1cd946a32c891861e5e
theStack:
ACK fafde92f84fb7c245bc3c1cd946a32c891861e5e
Tree-SHA512: 9ed2720b50d1c0938f30543ba143e1a4c6af3a0ff166f8b3eb452e1d99ddee6e3443a4c99f77efe94b8c3eb2feff984bf5259807ee8085e1e0e1e0d1de98227e
|
|
|
|
|
|
Also, remove wrong nChainTx comment and cast.
|
|
keys when inferring
74c77825e5ab68bfa575dad86444506c43ef6c06 test: Unit test for inferring scripts with hybrid and uncompressed keys (Andrew Chow)
f895f97014ac5fac46d27725c1ea7decf7ff76d4 test: Scripts with hybrid pubkeys are migrated to watchonly (Andrew Chow)
37b9b734770e855b9beff3b5085125f1420dd072 descriptors: Move InferScript's pubkey validity checks to InferPubkey (Andrew Chow)
b7485f11ab3a0f1860b261f222362af3301e0781 descriptors: Check result of InferPubkey (Andrew Chow)
Pull request description:
`InferDescriptor` was not always checking that the pubkey it was placing into the descriptor was an allowed pubkey. For example, given a P2WPKH script that uses an uncompressed pubkey, it would produce a `wpkh()` with the uncompressed key. Additionally, the hybrid key check was only being done for `pk()` scripts, where it should've been done for all scripts.
This PR moves the key checking into `InferPubkey`. If the key is not valid for the context, then `nullptr` is returned and the inferring will fall through to the defaults of either `raw()` or `addr()`.
This also resolves an issue with migrating legacy wallets that contain hybrid pubkeys as such watchonly scripts will become `raw()` or `addr()` and go to the watchonly wallet. Note that a legacy wallet cannot sign for hybrid pubkeys. A test has been added for the migration case.
Also added unit tests for `InferDescriptor` itself as the edge cases with that function are not covered by the descriptor roundtrip test.
ACKs for top commit:
furszy:
ACK 74c77825
Sjors:
utACK 74c77825e5ab68bfa575dad86444506c43ef6c06
Tree-SHA512: ed5f63e42a2e46120245a6b0288b90d2a6912860814c6c08fe393332add1cb364dc5eca72f16980352143570aef0c07bf1a91acd294099463bd028b6ce2fe40c
|
|
|
|
and check disk space periodically
ed52e71176fc97c6ed01e3eebd85acdec54b4448 Periodically check disk space to avoid corruption (Aurèle Oulès)
7fe537f7a48675b1d25542bee6f390d665547580 Implement CCoinsViewErrorCatcher::HaveCoin (Aurèle Oulès)
Pull request description:
Attempt to fix #26112.
As suggested by sipa in https://github.com/bitcoin/bitcoin/issues/26112#issuecomment-1249683401:
> CCoinsViewErrorCatcher, the wrapper class used around CCoinsViewDB that's supposed to detect these problems and forcefully exit the application, has an override for GetCoins. But in CheckTxInputs, HaveInputs is first invoked, which on its turn calls HaveCoin. HaveCoin is implemented in CCoinsViewDB, but not in CCoinsViewErrorCatcher, and thus the disk read exception escapes.
> A solution may be to just add an override for HaveCoin in CCoinsViewErrorCatcher.
I implemented `CCoinsViewErrorCatcher::HaveCoin` and also added a periodic disk space check that shutdowns the node if there is not enough space left on disk, the minimum here is 50MB.
For reviewers, it's possible to saturate disk space to test the PR by creating large files with `fallocate -l 50G test.bin`
ACKs for top commit:
achow101:
ACK ed52e71176fc97c6ed01e3eebd85acdec54b4448
w0xlt:
Code Review ACK https://github.com/bitcoin/bitcoin/pull/26331/commits/ed52e71176fc97c6ed01e3eebd85acdec54b4448
sipa:
utACK ed52e71176fc97c6ed01e3eebd85acdec54b4448
Tree-SHA512: 456aa7b996023df42b4fbb5158ee429d9abf7374b7b1ec129b21aea1188ad19be8da4ae8e0edd90b85b7a3042b8e44e17d3742e33808a4234d5ddbe9bcef1b78
|
|
|
|
|
|
InferPubkey can return a nullptr, so check it's result before continuing
with creating the inferred descriptor.
|
