If the hash is not noexcept, unorderd_map has to assume that it can throw an exception. Thus when rehashing care needs to be taken. libstdc++ solves this by simply caching the hash value, which increases memory of each node by 8 bytes. Adding noexcept prevents this caching. In my experiments with -reindex-chainstate -stopatheight=594000, memory usage has decreased by 9.4% while runtime has increased by 1.6% due to additional hashing. Additionally, memusage::DynamicUsage() is now more accurate and does not underestimate.
fa7847d99b gui: Rename address checkbox back to bech32 (MarcoFalke)
Pull request description:
This is the wording that has been used in the previous release, so translations should still exist for it.
Fixes: #16924
ACKs for top commit:
promag:
ACK fa7847d99b.
laanwj:
ACK fa7847d99b
Tree-SHA512: 0ac6c47fe5eb2145b609a30fd3f56052d3e08abe6c67fc74b6d209a55a4df509c52f13eb1c759520a4fa43916ece0e6d4cefef87e061b51114a6582db911944a
6659810e2f test: use named args for sendrawtransaction calls (Jon Atack)
5c1cd78b7e doc: improve rawtransaction code/test docs (Jon Atack)
acc14c5093 test: fix incorrect value in rpc_rawtransaction.py (Jon Atack)
Pull request description:
Follow-up to PR #16521.
- Fix incorrect value in rpc_rawtransaction test as per https://github.com/bitcoin/bitcoin/pull/16521/files#r325842308
- Improve the code docs
- Use named arguments as per https://github.com/bitcoin/bitcoin/pull/16521/files#r310715127
Happy to squash or keep only the first commit if the others are too fixup-y.
ACKs for top commit:
laanwj:
ACK 6659810e2f
Tree-SHA512: bf5258f23802ab3ba3defb8791097e08e63f3e2af21023f832cd270dc88d1fa04349e921d69f9f5fedac5dce5cd3c1cc46b48febbede4bc18dccb8be994565b2
6170ec5d3a Do not query all DNS seed at once (Pieter Wuille)
Pull request description:
Before this PR, when we don't have enough connections after 11 seconds, we proceed to query all DNS seeds in a fixed order, loading responses from all of them.
Change this to to only query three randomly-selected DNS seed. If 11 seconds later we still don't have enough connections, try again with another one, and so on.
This reduces the amount of information DNS seeds can observe about the requesters by spreading the load over all of them.
ACKs for top commit:
Sjors:
ACK 6170ec5d3
sdaftuar:
ACK 6170ec5d3a
jonasschnelli:
utACK 6170ec5d3a - I think the risk of a single seeder codebase is orthogonal to this PR. Such risks could also be interpreted differently (diversity could also increase the risk based on the threat model).
fanquake:
ACK 6170ec5d3a - Agree with the reasoning behind the change. Did some testing with and without `-forcednsseed` and/or a `peers.dat` and monitored the DNS activity.
Tree-SHA512: 33f6be5f924a85d312303ce272aa8f8d5e04cb616b4b492be98832e3ff37558d13d2b16ede68644ad399aff2bf5ff0ad33844e55eb40b7f8e3fddf9ae43add57
The test suite must always be defined (even when EVENT_SET_MEM_FUNCTIONS_IMPLEMENTED is not defined) so that the test harness doesn't fail due to not being able to find the raii_event_tests test.
This improves upon 95f97f4 actually fixing https://github.com/bitcoin/bitcoin/issues/9493
fa8d65f071 doc: Fix doxygen comment for SignTransaction in rpc/rawtransaction_util (MarcoFalke)
Pull request description:
The param `coins` to `SignTransaction` is final and can thus not be extended (as suggested by the doc).
ACKs for top commit:
practicalswift:
ACK fa8d65f071 -- const correctness is good and diff looks correct
fanquake:
ACK fa8d65f071
Tree-SHA512: 041e159f2c3cf96e296173c31f3e5f35bbc7711cc888aa4bf08aaa8c65c95ee7f7672f65396690a9af45795a618eea0fadde7fb02d29ec85f1b4df5e6d9e0c7a
3cf36736e5 refactoring: move ReplayBlocks under CChainState (James O'Beirne)
bcf73d3b84 refactoring: move LoadChainTip to CChainState method (James O'Beirne)
f5809d5b13 doc: fix CChainState::ActivateBestChain doc (James O'Beirne)
Pull request description:
This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11):
Parent PR: #15606
Issue: #15605
Specification: https://github.com/jamesob/assumeutxo-docs/tree/master/proposal
---
Move more chainstate-related functionality to methods on CChainState. Nothing too interesting here, but needed to work with multiple chainstates. And brief to review. :)
Also fixes doc on ActivateBestChain.
ACKs for top commit:
MarcoFalke:
ACK 3cf36736e5
ryanofsky:
Can confirm. utACK 3cf36736e5. Removes wrapper functions and removes more ::ChainActive() and ::ChainstateActive() calls than it adds, so seems good.
Tree-SHA512: 4bf8a1dd454ca9d61c85f6736910fa7354c57acc0002e3a8e5ce494035d8280e4c20e066f03478eeff7d44195e7912c282a486526da9be53854b478b961affaa
c4b0c08f7c Update tx-size-small comment with relevant CVE disclosure (Gregory Sanders)
Pull request description:
Code first introduced under https://github.com/bitcoin/bitcoin/pull/11423 with essentially no description and no discussion.
ACKs for top commit:
MarcoFalke:
ACK c4b0c08f7c
fanquake:
ACK c4b0c08f7c
Tree-SHA512: 95d5c92998b8b1e944c477dbaee265b62612b6e815099ab31d9ff580b4dff777abaf7f326a284644709f918aa1510412d62310689b1250ef6e64de7b19ca9f71
2dfd6834ef test: Add test for default maxfeerate in sendrawtransaction (Joonmo Yang)
261843e4be wallet/rpc: Use the default maxfeerate value as BTC/kB (Joonmo Yang)
Pull request description:
Fixes https://github.com/bitcoin/bitcoin/issues/16382
This patch tries to treat `maxfeerate` in sendrawtransaction/testmempoolaccept RPC as a rate(BTC/kB) instead of an absolute value(BTC).
The included test case checks if the new behavior works correctly, by using the transaction with an absolute fee of ~0.02BTC, where the fee rate is ~0.2BTC/kB.
This test should be failing if the default `maxfeerate` is 0.1BTC, but pass if the default value is 0.1BTC/kB
ACKs for top commit:
laanwj:
ACK 2dfd6834ef (ACKs by Sjors and MarcoFalke above for trivially different code)
Tree-SHA512: a1795bffe8a182acef8844797955db1f60bb0c0ded97148f3572dc265234d5219271a3a7aa0b6418a43f73b2b2720ef7412ba169c99bb1cdcac52051f537d6af
b4fd0ca9be Include cstring for sanity_test_fdelt if required (Ben Woosley)
7fb886b1b1 [moveonly] Split glibc sanity_test_fdelt out (Ben Woosley)
Pull request description:
SmartOS FD_ZERO is implemented in a way that requires
an external declaration of memcpy. We can not simply
include cstring in the existing file because
sanity_test_memcpy is attempting to replace memcpy.
Instead split glibc_sanity into fdelt and memcpy files,
and include <cstring> in glibc_sanity/fdelt.cpp.
Fixes#13581, see also #13619
ACKs for top commit:
laanwj:
Code review an lightly tested (but not on SmartOS) ACK b4fd0ca9be
Tree-SHA512: 231306da291ad9eca8ba91bea1e9c27b6c2e96e484d1602e1c2cf27761202f9287ce0bc19fefd000943d2b449d0e5929cd39e2f7e09cf930d89fa520228ccbec
c0b5d97103 Test that joinpsbts randomly shuffles the inputs (Andrew Chow)
6f405a1d3b Shuffle inputs and outputs after joining psbts (Andrew Chow)
Pull request description:
`joinpsbts` currently just adds the inputs and outputs in the order of that the PSBTs were provided. This makes it extremely easy to identify which outputs belong to which inputs. This PR changes that so that all of the inputs and outputs are shuffled in the joined transaction.
ACKs for top commit:
instagibbs:
utACK c0b5d97103
jonatack:
ACK c0b5d97103 modulo suggestions for later.
Tree-SHA512: 14a0b7aae07d92e6d2c76a3a3b228b481e1964cb7d34f97515bdda18e2ea05a9f97c5a22affc143b86ae8b95c3cb239849fb54219d65512bc2112264dca915c8
4a87c5cfdf [refactor] Rewrite AcceptToMemoryPoolWorker() using smaller parts (Suhas Daftuar)
Pull request description:
This is in preparation for re-using these validation components for a new version of AcceptToMemoryPool() that can operate on multiple transactions ("package relay").
ACKs for top commit:
MarcoFalke:
re-ACK 4a87c5cfdf (did the rebase myself and arrived at the same result, mod whitespace)
laanwj:
ACK 4a87c5cfdf
Tree-SHA512: b0495c026ffe06146258bace3d5e0c9aaf23fa65f89f258abc4af5980812e68e63a799f1d923e78ac1ee6bcafaf1222b2c2690a527df9b65dff7b48a013f154e
0c62e3aa73 New regression testing for CVE-2018-17144, CVE-2012-2459, and CVE-2010-5137. (lucash-dev)
38bfca6bb2 Added comments referencing multiple CVEs in tests and production code. (lucash-dev)
Pull request description:
This functional test includes two scenarios that test for regressions of vulnerabilities, but they are only briefly described. There are freely available documents explaining in detail the issues, but without explicit mentions, the developer trying to maintain the code needs an additional step of digging in commit history and PR conversations to figure it out.
Added comments to explicitly mention CVE-2018-17144 and CVE-2012-2459, for more complete documentation.
This improves developer experience by making understanding the tests easier.
ACKs for top commit:
laanwj:
ACK 0c62e3aa73, checked the CVE numbers, thanks for adding documentation
Tree-SHA512: 3ee05351745193b8b959e4a25d50f25a693b2d24b0732ed53cf7d5882df40b5dd0f1877bd5c69cffb921d4a7acf9deb3cc1160b96dc730d9b5984151ad06b7c9
8573429d46 test: add some unit tests for merkle.cpp (soroosh-sdi)
Pull request description:
following situations are covered:
- empty block
- one Tx
- Merkle root of a block with odd Txs should not change with repeating
last one
- Merkle root is computed with combining Merkle root of left subtree and right subtree
- block witness is Merkle root of a block when setting first Tx
to zero.
Signed-off-by: soroosh-sdi <soroosh.sardari@gmail.com>
ACKs for top commit:
laanwj:
ACK 8573429d46
Tree-SHA512: e12228171de8f8480f173c9d9d0359f00f46bf09075e0767f5f1a367478a1b7b6d177d230f7e930914915cd2c6b66b18d24b1682f1233c38e97954ba331e5773
The motivation for this addition was to unit test the function
wallet.cpp:ExtractPubKey() (see recent change in commit
798a589aff) which is however static and only
indirectly available via the public methods AddWatchOnly(), LoadWatchOnly() and
RemoveWatchOnly(). Since the first of those methods also stores the addresses
to the disk, the second, simpler one was chosen which only operates in memory.
test: add missing wallet lock for test case WatchOnlyPubKeys
test: test case WatchOnlyPubKeys, suggested review changes by instagibbs
test: test case WatchOnlyPubKeys, suggested review changes by achow101
test: test case WatchOnlyPubKeys, s/isPubKeyFullyValid/is_pubkey_fully_valid
following situations are covered:
- empty block
- one Tx
- Merkle root of a block with odd Txs should not change with repeating
last one
- Merkle root is computed with combining Merkle root of left subtree and right subtree
- block witness is Merkle root of a block when setting first Tx
to zero.
Signed-off-by: soroosh-sdi <soroosh.sardari@gmail.com>
This is in preparation for re-using these validation components for a new
version of AcceptToMemoryPool() that can operate on multiple transactions
("package relay").
82e53f37e1 doc: add comments clarifying how local services are advertised (James O'Beirne)
Pull request description:
Recent questions have come up regarding dynamic service registration
(see https://github.com/bitcoin/bitcoin/pull/16442#discussion_r308702676
and the assumeutxo project ~~which needs to dynamically flip NODE_NETWORK~~).
While investigating how dynamic service registration might work, I was
confused about how we convey local services to peers. This adds some
documentation that hopefully clarifies this process.
ACKs for top commit:
laanwj:
ACK 82e53f37e1
darosior:
ACK 82e53f37e1
Tree-SHA512: a30c1020387d7a75b5b3cdde45f8b7f2ae46293da97e6227b2ee17e290b93deb5b16c0bbc2b1676972300e5c3c2ad74eb8b3910d6b93e028dac1ae2700468ef9
fa912a8ad5 doc: move-only ActivateBestChain doxygen comment to header (MarcoFalke)
fa99efd054 doc: ActivateBestChainStep return value (MarcoFalke)
Pull request description:
It will always return true, unless a system error such as #15305 occurred
ACKs for top commit:
laanwj:
ACK fa912a8ad5
Tree-SHA512: d439da844a467f9705014b946d7d987fb62cb63fe6a325b2fdbbb73a6578fc0ade3f60892044f02face43948204fc4e3c9fa70d108233d4ca8eef27984059689
cad3ab5db8 gui: fix autofocus in CreateWalletActivity::askPassphrase() (Jon Atack)
539d9403af gui: fix passphrase labels/tooltip in createwalletdialog/askpassphrasedialog (Jon Atack)
43aa9b0d79 gui: rename encrypt(), blank(), and askPasshprase() (Jon Atack)
Pull request description:
Closes#16820. The wallet [name escaping issue](https://github.com/bitcoin/bitcoin/pull/15450#pullrequestreview-282297760) in that issue predates #15450 and is fixed by #16826.
- [x] rename encrypt() to encryptWallet(), and blank() to makeBlankWallet() // EDIT: updated to
isEncryptWalletChecked()
isDisablePrivateKeysChecked()
isMakeBlankWalletChecked()
- [x] fix naming of askPasshprase() to askPassphrase()
- [x] fix passphrase labels and tooltip in createwalletdialog.ui and askpassphrasedialog.ui
- [x] fix grammar of labels in askpassphrase dialog and WalletController::closeWallet
- [x] fix autofocus in CreateWalletActivity::askPassphrase()
Squashed down to three commits.
Reviewers, to test manually: build, launch the gui wallet, and look at labels/tooltips/focus with the create wallet, encrypt wallet, change password, and close wallet commands.
ACKs for top commit:
jb55:
Approach ACK cad3ab5db8
instagibbs:
code review and tACK cad3ab5db8
fanquake:
ACK cad3ab5db8
Tree-SHA512: b441fbf8f8cd370dd692bac24f0d3c1b32fc7d947b6c3a2c9ba7cf0bc175a72b3460440f2f10f7632c0e8e0f8e65fe15615a30c46e2c7763bf258c504b457dd6
b6233a4985 bitcoin-wallet: Add a missing closing parenthesis in the help (darosior)
Pull request description:
ACKs for top commit:
kristapsk:
utACK b6233a4985
fanquake:
ACK b6233a4985
Tree-SHA512: acf18633fdca4bd73838fcaa0ebe4121dd0b5308daa77c4458ec4c98a9e8aa6d9d6580a48c884147438af14e670b0606c1e76f72d1d7efd221c4da419061beed
1b41c2c8a1 test: improve gettransaction test coverage (Jon Atack)
0f34f54888 rpc: fix regression in gettransaction (Jon Atack)
Pull request description:
Closes#16872.
PR #16866 renamed the `decode` argument in gettransaction to `verbose` to make it more consistent with other RPC calls like getrawtransaction. However, it inadvertently overloaded the "details" field when `verbose` is passed. The result is that the original "details" field is no longer returned correctly, which seems to be a breaking API change.
This PR:
- takes the simplest path to restoring the "details" field by renaming the decoded one back to "decoded" while leaving the `verbose` argument for API consistency, which was the main intent of #16866,
- addresses [this comment](https://github.com/bitcoin/bitcoin/pull/16185#discussion_r320740413) by mentioning in the RPC help that the new decoded field is equivalent to decoderawtransaction, and
- updates the help, functional test, and release note.
Reviewers, to test this manually, build and run `bitcoin-cli help gettransaction` and `bitcoin-cli gettransaction <wallet txid> false true`, and verify that the command returns both `details` and `decoded` fields.
ACKs for top commit:
jnewbery:
tACK 1b41c2c8a1
Tree-SHA512: 287edd5db7ed58fe8b548975aba58628bd45ed708b28f40174f10a35a455d89f796fbf27430aa881fc376f47aabda8803f74d4d100683bd86577a02279091cf3
1153caf78e Qt: advise users not to switch wallets when opening a BIP70 URI. (James Hilliard)
Pull request description:
It would probably be a good idea to have something like this before #15584 is merged.
ACKs for top commit:
jonasschnelli:
utACK 1153caf78e
fanquake:
ACK 1153caf78e
Tree-SHA512: 6e682dd280c44eaafb1206c32439df42a20173c33297bf93dd607f0a7a2faec8e2d17fff83c85027083ebd11a71795b443e707992251574370dd1d46b7bff060
PR 16866 renamed the 'decode' argument in gettransaction to 'verbose' to make it more consistent with other RPC calls like getrawtransaction.
However, it seems it inadvertently overloaded the 'details' fields when 'verbose' is passed. The result is that the original 'details' fields are no longer returned, which seems to be a breaking API change.
This PR takes the simplest path to restoring the 'details' fields by renaming them from 'details' back to 'decoded', while leaving the 'verbose' argument for API consistency.
It also addresses [this comment](https://github.com/bitcoin/bitcoin/pull/16185#discussion_r320740413) to mention that the 'decoded' field is identical to decoderawtransaction.
Update the RPC help, functional test, and release note.
This makes the RPC method consistent with other RPC methods that have a
'verbose' option.
Change the name of the return object from 'decoded' to details.
Update help text.
9924bce317 [gui] intro: enable pruning by default unless disk is big (Sjors Provoost)
c8de347a9d [gui] intro: add prune preference (Sjors Provoost)
1bbc49d207 [gui] intro: inform caller if intro was shown (Sjors Provoost)
1957103786 [gui] add explicit prune setter (Sjors Provoost)
1bccf6a52d [node] add forceSetArg to interface (Sjors Provoost)
Pull request description:
This adds a checkbox to the intro screen to enable pruning from the get go.
If the user has plenty of space, it's unchecked by default:
<img width="671" alt="big" src="https://user-images.githubusercontent.com/10217/63641289-10339000-c6ac-11e9-98d7-caf64dff0da6.png">
If the user has insufficient space it's checked by default:
<img width="897" alt="low" src="https://user-images.githubusercontent.com/10217/63641276-d4002f80-c6ab-11e9-9f5b-a53472f814ff.png">
When the user has barely enough space and is likely to need pruning in the near future, this is shown in yellow and we also check the prune box:
<img width="662" alt="medium" src="https://user-images.githubusercontent.com/10217/63641294-1c1f5200-c6ac-11e9-8ecb-6b69e42b1ece.png">
The cut-off for this 10 GB above `m_assumed_blockchain_size` (`=240` in `chainparams.cpp`).
If the user launches the first time with `-prune=...` then we disable the check box and display the correct size (rounded to GB):
<img width="658" alt="Schermafbeelding 2019-08-24 om 20 23 14" src="https://user-images.githubusercontent.com/10217/63641351-09594d00-c6ad-11e9-94fe-fe5ed562e109.png">
The 2 GB default matches the settings default. The user can't change it in the intro screen, but can change it later. I'm tempted to increase that default to 10 GB, and then have the intro screen reduce it if space is really tight.
Tips for testing:
* move your existing data dir elsewhere
* wipe data dir at every restart (behavior is different if it exists)
* launch with `bitcoin-qt -resetguisettings -lang=en` (there's some space issues in different languages)
* fake your free space by changing `intro.cpp` line 90: `freeBytesAvailable = 5000000000; // 5 GB`
* try both testnet and mainnet, because settings are seperate. In particular note how step 7 in `GuiMain` switches where `QTSettings settings` points to; this had me thoroughly confused on testnet, because I was setting them too early.
ACKs for top commit:
jonasschnelli:
Tested ACK 9924bce317
ryanofsky:
utACK 9924bce317. The changes are very logical, and implement the feature in a clean that way that doesn't add a lot of complication and shouldn't interfere with future improvements. I looked at Luke's branch too, and I think there's also a lot of great stuff there that seems fully compatible with this change.
Tree-SHA512: 9523961451c53aebd347716976bc3a4a398f989dc21e9bbbd357060bd11a8f46c435f068bd421bb31ccb08e55445ef67bc347d8d19a4fb8fde9d6d3f9a3bcbb0
