Commit Graph

30181 Commits

Author SHA1 Message Date
Samuel Dobson
ce0913148b
Merge bitcoin/bitcoin#22547: cli: Add progress bar for -getinfo
b851a92c06 cli: Add progress bar for -getinfo (klementtan)

Pull request description:

  Add a progress bar for the `Verification progress` attribute in `-getinfo` when verification progress `< 99%`.

  ![image](https://user-images.githubusercontent.com/49265907/127897458-27d8aaa9-7893-4665-9c40-36389a8d9cbb.png)

  **Motivation**:
  * Improve user-friendliness of `-getinfo`
  * Can be useful with `watch -n 1 bitcoin-cli -getinfo`(suggested by theStack [below](https://github.com/bitcoin/bitcoin/pull/22547#issuecomment-887488172))
  * The progress bar is only display when are still syncing to tip(verification progress `< 99%`)

  **Reviewing**

  If your verification progress is `> 99%` you can restart the verification progress with

  ```shell
  $ ./src/bitcoind -reindex
  $./src/bitcoin-cli -getinfo
  ```

ACKs for top commit:
  prayank23:
    reACK b851a92c06
  theStack:
    re-ACK b851a92c06 🍹
  Zero-1729:
    re-tACK b851a92c06 (re-tested, works as expected 🍾)
  jonatack:
    ACK b851a92c06
  lsilva01:
    Tested ACK b851a92c06 on mainnet and signet on Ubuntu 20.04.

Tree-SHA512: 2046d812e3c4623c6cc3ed4c24f2daaa92ba12cd181fa21626b782743890c2373be3175cff1441a7ba37295b6d5818368deea90d483959875c22f7ad9b601a20
2021-08-10 22:26:10 +12:00
fanquake
8193294cab
Merge bitcoin/bitcoin#22642: release: Release with separate SHA256SUMS and sig files
90b3e482e9 release: Release with separate SHA256SUMS and sig files (Carl Dong)

Pull request description:

  This allows us to:
  - remove the rfc4880 EOL hacks, and
  - release with a SHA256SUMS.asc file that's a combination of all signer signatures

ACKs for top commit:
  achow101:
    ACK 90b3e482e9
  laanwj:
    Concept and code review ACK 90b3e482e9

Tree-SHA512: 5d5086063d303aa0cbd590e5fdf2ae8f555e25f4e43bf67545e33384449b990e94834c711622530ad0eb3dcc83f52746884a5081dadb0acff8dd799cfadafac7
2021-08-09 16:19:49 +08:00
fanquake
a9509fedc8
Merge bitcoin/bitcoin#22643: guix-verify: Non-zero exit code when anything fails
d451b60d22 guix-verify: Non-zero exit code when anything fails (Carl Dong)

Pull request description:

  ```
  Previously, if verification fails, the correct message will be printed,
  but the exit code would still be 0.
  ```

ACKs for top commit:
  achow101:
    ACK d451b60d22

Tree-SHA512: 695d72121f308d8a66db780eca16878fb378b4d766de5b58c2d6f778c5661a2f7bdf37a96e8e8e283b0a46b5d55f24bca05fa7509aa1822f0854c50064200572
2021-08-09 14:24:43 +08:00
fanquake
21438d55d5
Merge bitcoin/bitcoin#21800: mempool/validation: mempool ancestor/descendant limits for packages
accf3d5868 [test] mempool package ancestor/descendant limits (glozow)
2b6b26e57c [test] parameterizable fee for make_chain and create_child_with_parents (glozow)
313c09f7b7 [test] helper function to increase transaction weight (glozow)
f8253d69d6 extract/rename helper functions from rpc_packages.py (glozow)
3cd663a5d3 [policy] ancestor/descendant limits for packages (glozow)
c6e016aa13 [mempool] check ancestor/descendant limits for packages (glozow)
f551841d3e [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits (glozow)
97dd1c729d MOVEONLY: add helper function for calculating ancestors and checking limits (glozow)
f95bbf58aa misc package validation doc improvements (glozow)

Pull request description:

  This PR implements a function to calculate mempool ancestors for a package and enforces ancestor/descendant limits on them as a whole. It reuses a portion of `CalculateMemPoolAncestors()`; there's also a small refactor to move the reused code into a generic helper function. Instead of calculating ancestors and descendants on every single transaction in the package and their ancestors, we use a "worst case" heuristic, treating every transaction in the package as each other's ancestor and descendant. This may overestimate everyone's counts, but is still pretty accurate in the our main package use cases, in which at least one of the transactions in the package is directly related to all the others (e.g. 1 parent + 1 child, multiple parents with 1 child, or chains).

  Note on Terminology: While "package" is often used to describe groups of related transactions _within_ the mempool, here, I only use package to mean the group of not-in-mempool transactions we are currently validating.

  #### Motivation

  It would be a potential DoS vector to allow submission of packages to mempool without a proper guard for mempool ancestors/descendants. In general, the purpose of mempool ancestor/descendant limits is to limit the computational complexity of dealing with families during removals and additions. We want to be able to validate multiple transactions on top of the mempool, but also avoid these scenarios:

  - We underestimate the ancestors/descendants during package validation and end up with extremely complex families in our mempool (potentially a DoS vector).
  - We expend an unreasonable amount of resources calculating everyone's ancestors and descendants during package validation.

ACKs for top commit:
  JeremyRubin:
    utACK accf3d5
  ariard:
    ACK accf3d5.

Tree-SHA512: 0d18ce4b77398fe872e0b7c2cc66d3aac2135e561b64029584339e1f4de2a6a16ebab3dd5784f376e119cbafc4d50168b28d3bd95d0b3d01158714ade2e3624d
2021-08-09 12:23:39 +08:00
Samuel Dobson
b1a672d158
Merge bitcoin/bitcoin#22337: wallet: Use bilingual_str for errors
92993aa5cf Change SignTransaction's input_errors to use bilingual_str (Andrew Chow)
171366e89b Use bilingual_str for address fetching functions (Andrew Chow)
9571c69b51 Add bilingual_str::clear() (Andrew Chow)

Pull request description:

  In a couple of places in the wallet, errors are `std::string`. In order for these errors to be translated, change them to use `bilingual_str`.

ACKs for top commit:
  hebasto:
    re-ACK 92993aa5cf, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22337#pullrequestreview-694542729) review, verified with
  klementtan:
    Code review ACK 92993aa5cf
  meshcollider:
    Code review ACK 92993aa5cf

Tree-SHA512: 5400e419dd87db8c49b67ed0964de2d44b58010a566ca246f2f0760ed9ef6a9b6f6df7a6adcb211b315b74c727bfe8c7d07eb5690b5922fa5828ceef4c83461f
2021-08-09 14:45:12 +12:00
Samuel Dobson
a162edfdd1
Merge bitcoin/bitcoin#22359: wallet: Do not set fInMempool in transactionAddedToMempool when tx is not in the mempool
fa6fd3dd6a wallet: Properly set fInMempool in mempool notifications (MarcoFalke)

Pull request description:

  A wallet method (like bumping the fee) might have set `fInMempool` to false because the transaction was removed from the mempool (See commit fa4e088cba).

  Avoid setting it back to true (incorrectly) in the validation interface background thread.

  Fixes #22357

ACKs for top commit:
  ryanofsky:
    Code review ACK fa6fd3dd6a. Only change since last review is extending workaround to `transactionRemovedFromMempool`. Since we know this workaround is imperfect and the goal of this PR is mainly to fix CI errors, I would probably be inclined to limit the workaround to as few places as possible where we have seen actual failures, instead of adding the workaround to as many places as possible, where there is some chance it might trigger new failures. But since this workaround is so straightforward and almost looks like a real fix, probably it doesn't matter.
  meshcollider:
    utACK fa6fd3dd6a

Tree-SHA512: d690136a577f1f532aa1fee80d3f6600ff7fc61286fbf564a53d7938d5ae52d33f0dbb0fef8b8c041a4970fb424f0b9f1ee7ce791e0ff8354e0000ecc9e22b84
2021-08-09 14:21:22 +12:00
Samuel Dobson
8fa03c4ddf
Merge bitcoin/bitcoin#21500: wallet, rpc: add an option to list private descriptors
bb822a7af8 wallet, rpc: add listdescriptors private option (S3RK)

Pull request description:

  Rationale: make it possible to backup your wallet with `listdescriptors` command

  * The default behaviour is still to show public version
  * For private version only the root xprv is returned

  Example use-case:
  ```
  > bitcoin-cli -regtest -named createwallet wallet_name=old descriptors=true
  > bitcoin-cli -regtest -rpcwallet=old listdescriptors true | jq '.descriptors' > descriptors.txt

  > bitcoin-cli -regtest -named createwallet wallet_name=new descriptors=true blank=true
  > bitcoin-cli -regtest -rpcwallet=new importdescriptors "$(cat descriptors.txt)"
  ```

  In case of watch-only wallet without private keys there will be following output:
  ```
  error code: -4
  error message:
  Can't get descriptor string.
  ```

ACKs for top commit:
  achow101:
    re-ACK bb822a7af8
  Rspigler:
    tACK bb822a7af8
  jonatack:
    ACK bb822a7af8 per `git diff 2854ddc bb822a7`
  prayank23:
    tACK bb822a7af8
  meshcollider:
    Code review ACK bb822a7af8

Tree-SHA512: f6dddc72a74e5667071ccd77f8dce578382e8e29e7ed6a0834ac2e114a6d3918b59c2f194f4079b3259e13d9ba3b4f405619940c3ecb7a1a0344615aed47c43d
2021-08-09 14:09:07 +12:00
MarcoFalke
db94d74f24
Merge bitcoin/bitcoin#22657: fuzz: Re-enable assert in banman again
fabed982ad fuzz: Re-enable assert in banman again (MarcoFalke)

Pull request description:

  Looks like this was accidentally fixed by removing the buggy code in commit efd6f904c7

ACKs for top commit:
  hebasto:
    ACK fabed982ad

Tree-SHA512: 2dea5dad48ff2050ae7086c1c6306c40f650e9629918e79adc54164a375d777a70b29f5a480566dc6430f07ce33dfe703fc5d45a20125584b4a026c5832198a2
2021-08-07 16:28:50 +02:00
MarcoFalke
fabed982ad
fuzz: Re-enable assert in banman again 2021-08-07 10:21:45 +02:00
Hennadii Stepanov
03826aecc5
Merge bitcoin-core/gui#396: Ensure external signer option remains disabled without signers
a9b9ca82da gui: ensure external signer option remains disabled without signers (Andrew Chow)

Pull request description:

  When no external signers are available, the option to enable external signers should always be disabled. However the encrypt wallet checkbox can erroneously re-enable the external signer checkbox. To avoid this, CreateWalletDialog now stores whether signers were available during setSigners so that future calls to external_signer_checkbox->setEnabled can account for whether signers are available.

  Fixes #395

ACKs for top commit:
  hebasto:
    ACK a9b9ca82da, tested on Linux Mint 20.2 (Qt 5.12.8).
  Sjors:
    tACK a9b9ca82da
  jarolrod:
    ACK a9b9ca82da

Tree-SHA512: 98951bcadc23fce99a66ea2d367c44360989e888c253845a767e1f7085c594562d0f099de4130f4a078c5072aa7806294097d976ee6407291f3d3c5a4a608b44
2021-08-06 19:17:09 +03:00
Hennadii Stepanov
7ebc4c6689
Merge bitcoin-core/gui#379: Prompt to reset settings when settings.json cannot be read
1ee6d0b01a gui: Prompt to reset settings when settings.json cannot be read (Russell Yanofsky)

Pull request description:

  Currently the GUI shows confusing error messages when `settings.json` can't be read or written on startup. This causes the unrecoverable read error described in bitcoin/bitcoin#21340 and write error described bitcoin/bitcoin#21974. Current error read message looks like:

  ![current](https://user-images.githubusercontent.com/7133040/124977362-638ffc80-dffe-11eb-9edd-89135a9bc602.png)

  This PR tries to clarify the error dialog, and adds an option to just clear the settings and reset them to default:

  ![new-read-error](https://user-images.githubusercontent.com/7133040/124977636-b669b400-dffe-11eb-8d35-02eda95f48c0.png)
  ![new-read-details](https://user-images.githubusercontent.com/7133040/124977644-bb2e6800-dffe-11eb-9209-11c1c3d7be40.png)

  Additionally the PR also shows a slightly better error message when there is an error trying to write the settings file. This error probably should occur less frequently, but it is easy to improve, and it should be good to make the write error consistent with the read error. The new write error dialog looks like:

  ![new-write-error](https://user-images.githubusercontent.com/7133040/124978016-3bed6400-dfff-11eb-9d79-9b2e9bbc4369.png)

  ![new-write-details](https://user-images.githubusercontent.com/7133040/124978025-3db72780-dfff-11eb-8df5-741f75a402d9.png)

ACKs for top commit:
  jarolrod:
    ACK 1ee6d0b01a
  Zero-1729:
    ACK 1ee6d0b01a
  hebasto:
    ACK 1ee6d0b01a, tested on Linux Mint 20.2 (Qt 5.12.8).

Tree-SHA512: fb57a0a0d032e3f8219fff49a4de69b4c962bf0b448544ccf9d8d4d45c5bd209e23653d4f13300b9e534b9c03de159498bef1658e95defe3ab6a8ecac57d592c
2021-08-06 18:44:14 +03:00
glozow
accf3d5868 [test] mempool package ancestor/descendant limits 2021-08-06 10:04:59 +01:00
glozow
2b6b26e57c [test] parameterizable fee for make_chain and create_child_with_parents 2021-08-06 10:04:59 +01:00
glozow
313c09f7b7 [test] helper function to increase transaction weight 2021-08-06 10:04:59 +01:00
glozow
f8253d69d6 extract/rename helper functions from rpc_packages.py
MOVEONLY; no change in behavior. Rename because there is another helper
funciton in chain_transaction in test_framework.util.py
2021-08-06 10:04:59 +01:00
glozow
3cd663a5d3 [policy] ancestor/descendant limits for packages 2021-08-06 10:04:59 +01:00
Andrew Chow
a9b9ca82da gui: ensure external signer option remains disabled without signers
When no external signers are available, the option to enable external
signers should always be disabled. However the encrypt wallet checkbox
can erroneously re-enable the external signer checkbox. To avoid this,
CreateWalletDialog now stores whether signers were available during
setSigners so that future calls to external_signer_checkbox->setEnabled
can account for whether signers are available.
2021-08-05 23:27:53 -04:00
Carl Dong
d451b60d22 guix-verify: Non-zero exit code when anything fails
Previously, if verification fails, the correct message will be printed,
but the exit code would still be 0.
2021-08-05 19:05:16 -04:00
Carl Dong
90b3e482e9 release: Release with separate SHA256SUMS and sig files
This allows us to remove the rfc4880 EOL hacks and release with a
SHA256SUMS.asc file that's a combination of all signer signatures.
2021-08-05 16:58:54 -04:00
MarcoFalke
d67330d112
Merge bitcoin/bitcoin#21129: fuzz: check that ser+unser produces the same AddrMan
87651795d8 fuzz: check that ser+unser produces the same AddrMan (Vasil Dimov)
6408b24517 fuzz: move init code to the CAddrManDeterministic constructor (Vasil Dimov)

Pull request description:

  Add a fuzz test that fills addrman with a pile of randomly generated addresses, serializes it to a stream, unserializes the stream to another addrman object and compares the two.

  Some discussion of this already happened at https://github.com/jnewbery/bitcoin/pull/18.

ACKs for top commit:
  practicalswift:
    cr ACK 87651795d8
  jonatack:
    ACK 87651795d8 rebased to current master, reviewed, fuzz build, ran `FUZZ=addrman_serdeser src/test/fuzz/fuzz`

Tree-SHA512: 7eda79279f14f2649840bf752e575d7b02cbaad541f74f7254855ebd4a32da988f042d78aa9228983350283bb74dd0c71f51f04c0846889c3ba2f19f01a0c303
2021-08-05 15:17:50 +02:00
glozow
c6e016aa13 [mempool] check ancestor/descendant limits for packages
When calculating ancestor/descendant counts for transactions in the
package, as a heuristic, count every transaction in the package as an
ancestor and descendant of every other transaction in the package.

This may overestimate, but will not underestimate, the
ancestor/descendant counts. This shortcut still produces an accurate
count for packages of 1 parent + 1 child.
2021-08-05 12:37:28 +01:00
glozow
f551841d3e [refactor] pass size/count instead of entry to CalculateAncestorsAndCheckLimits
This does not change existing behavior.
The ancestor/descendant limits are inclusive of the entries themselves,
but CalculateAncestorsAndCheckLimits() does not need access to them.
2021-08-05 12:37:28 +01:00
glozow
97dd1c729d MOVEONLY: add helper function for calculating ancestors and checking limits 2021-08-05 12:37:28 +01:00
glozow
f95bbf58aa misc package validation doc improvements 2021-08-05 12:37:28 +01:00
MarcoFalke
f4328ebef5
Merge bitcoin/bitcoin#22619: test: refactor: use consistent bytes <-> hex-string conversion in functional test framework
5a1bef60a0 test: refactor: remove binascii from test_framework (Zero-1729)

Pull request description:

  This PR continues the work started in PR #22593, regarding using the `bytes` built-in module. In this PR specifically, instances of `binascii`'s methods `hexlify`, `unhexlify`,  and `a2b_hex` have been replaced with the build-in `bytes` module's `hex` and `fromhex` methods where appropriate to make bytes <-> hex-string conversions consistent across the functional test files and test_framework.

  Additionally, certain changes made are based on the following assumption:

  ```
  bytes.hex(data) == binascii.hexlify(data).decode()
  bytes.hex(data).encode() == binascii.hexlify(data)
  ```

  Ran the functional tests to ensure behaviour is still consistent and changes didn't break existing tests.

  closes #22605

ACKs for top commit:
  theStack:
    Code-review ACK 5a1bef60a0 🔢

Tree-SHA512: 8f28076cf0580a0d02a156f3e1e94c9badd3d41c3fbdfb2b87cd8a761dde2c94faa5f4c448d6747b1ccc9111c3ef1a1d7b42a11c806b241fa0410b7529e2445f
2021-08-05 12:15:01 +02:00
fanquake
c4b42aa4ff
Merge bitcoin/bitcoin#22630: test: Add missing include
6666ec9e05 test: Add missing include (MarcoFalke)

Pull request description:

  Fix the silent merge conflict introduced in commit 7d60f7ec6b

ACKs for top commit:
  fanquake:
    ACK 6666ec9e05 - fixes:

Tree-SHA512: c712715db4a41cf6ed991fb35482d8fd9016b5a27e1309654018219ebbfd8d4f98a389bdf5d7b792e7a4690928aeef943c716465b0c5e00f70320ad2e134ebb5
2021-08-05 16:00:54 +08:00
MarcoFalke
6666ec9e05
test: Add missing include 2021-08-05 09:53:03 +02:00
MarcoFalke
7d60f7ec6b
Merge bitcoin/bitcoin#22277: test: Properly set BIP34 height in CreateNewBlock_validity unit test
faa670d386 test: Properly set BIP34 height in CreateNewBlock_validity unit test (MarcoFalke)

Pull request description:

  The coinbase scriptSig in this unit test has several issues:

  * The BIP34 height is not the "first item" as required (See https://github.com/bitcoin/bips/blob/master/bip-0034.mediawiki#specification)
  * It uses the wrong encoding ( See da69d9965a/src/validation.cpp (L3250) )
  * It uses the wrong height (off by one)

  While BIP34 isn't currently enforced in this unit test, this should be fixed to avoid confusion and to promote self-consistency.

  The change obviously requires new proof of work (`BLOCKINFO`).

  Also change the block version from `1` to `VERSIONBITS_TOP_BITS`, because this test shouldn't care about the block version and bumping it is required for other changes.

ACKs for top commit:
  theStack:
    Code review ACK faa670d386

Tree-SHA512: 8dbe2d5300a640f3e1817ff048906e60463aca64ba50fec8ee4f18fb1c70e511008755b0b5baba81114a1a6265fdfae9a4b7ae8acadfb2c7ad43223157a0386c
2021-08-05 09:40:52 +02:00
MarcoFalke
dd981b5e84
Merge bitcoin/bitcoin#22618: [p2p] Small follow-ups to 21528
9778b0fec1 [net_processing] Provide debug error if code assumptions change. (Amiti Uttarwar)
aa79c91260 [docs] Add release notes for #21528 (Amiti Uttarwar)

Pull request description:

  Adds a release note & addresses [this](https://github.com/bitcoin/bitcoin/pull/21528#discussion_r680963101) review comment to make expectations more explicit.

ACKs for top commit:
  Zero-1729:
    re-ACK 9778b0fec1
  jonatack:
    ACK 9778b0fec1

Tree-SHA512: 9507df5f2746d05c6df8c86b7a19364610ebfafc81af7650be7e68d7536a0685cce9fd2e5f287ef92b6245c584f8875b24a958109ba5bd8acf3c8fc9fd19eef2
2021-08-05 09:29:54 +02:00
Hennadii Stepanov
e9472e6828
Merge bitcoin-core/gui#393: Fix regression in "Encrypt Wallet" menu item
d54d949598 qt: Fix regression in "Encrypt Wallet" menu item (Hennadii Stepanov)

Pull request description:

  Fix #392.

  Adding a new item to the `m_wallet_selector` must follow the establishment of a connection between the `WalletView::encryptionStatusChanged` signal and the `BitcoinGUI::updateWalletStatus` slot.

  This was a regression introduced in 20e2e24e90 (#29).

  ---

  An _encrypted_ wallet being auto-loaded at the GUI startup:
  - on master (eaf09bda4a)

  ![Screenshot from 2021-08-03 22-38-49](https://user-images.githubusercontent.com/32963518/128075837-cdbb2047-5327-43ea-b2d5-2dcdef67cdc0.png)

  - with this PR

  ![Screenshot from 2021-08-03 22-34-58](https://user-images.githubusercontent.com/32963518/128075572-cb727652-ad44-4b85-bf64-edcd19f9dea1.png)

ACKs for top commit:
  achow101:
    ACK d54d949598
  jarolrod:
    ACK d54d949598

Tree-SHA512: 669615ec8e1517c2f4cdf59bd11a7c85be793ba0dda112361cf95e6c2f0636215fed331d26a86dc9b779a49defae1b248232f98dab449584376c111c288e87bb
2021-08-05 09:48:54 +03:00
Hennadii Stepanov
be37037e8e
Merge bitcoin-core/gui#337: test: Use Regex Search in Apptests
6969b2bb98 qt, test: use regex search in apptests (Jarol Rodriguez)
d09d1cf1a2 qt, test: introduce FindInConsole function (Jarol Rodriguez)

Pull request description:

  This PR refactors our GUI `apptests` so that it uses regex search to find values in our console/qtextedit output regardless if it is in `plaintext`, `html`, or `markdown`.

  This introduces a new function `FindInConsole` which uses [QRegularExpression](https://doc.qt.io/qt-5/qregularexpression.html) to search the output of the console. The function must be provided with a [perl compatible regex](https://www.debuggex.com/cheatsheet/regex/pcre) pattern which wants to match a single group. The function then returns the matched group. If no match is found, an empty `QString` is returned.

  We then use this new function in `TestRpcCommand` to find the current `chain` value instead of reading with univalue.

  This approach can apply to a wider variety of testing scenarios as we can reuse this function to search for values when the console output is exported in a different format than `plaintext`. As an example, A follow up PR will add tests for console resizing and needs to look for the size in `html` tags after exporting the console text with `toHtml()`.

ACKs for top commit:
  hebasto:
    ACK 6969b2bb98
  ShaMan239:
    ACK 6969b2bb98

Tree-SHA512: 4db8bcd4a1acc4539ca64bbd7de572fe7dd6afc3e95108235abfc2891585bc4db3a56a33928fa38e8d44ac87023ce0dee3abcfadfbcd4440e3a21a52fef02536
2021-08-05 08:53:03 +03:00
Amiti Uttarwar
9778b0fec1 [net_processing] Provide debug error if code assumptions change.
Currently, this call to SetupAddressRelay will never return false because of
the previous guard that returns early if the peer is not an inbound connection.
Rather than implicitly relying on this guarantee, throw an error in the debug
build if it ever changes.
2021-08-04 12:36:22 -07:00
Amiti Uttarwar
aa79c91260 [docs] Add release notes for #21528
And fix a typo in the test.
2021-08-04 12:36:22 -07:00
Zero-1729
5a1bef60a0
test: refactor: remove binascii from test_framework 2021-08-04 19:59:13 +01:00
MarcoFalke
4f1a75b1aa
Merge bitcoin/bitcoin#22621: make ParseOutputType return a std::optional<OutputType>
32fa49a184 make ParseOutputType return a std::optional<OutputType> (fanquake)

Pull request description:

  Similar to #22220. Skipped using `auto` here for the same reasons outlined in that PR.

ACKs for top commit:
  jnewbery:
    utACK 32fa49a184
  jonatack:
    Code review ACK 32fa49a184 and debian clang 13 debug build is clean / unit tests locally are green
  MarcoFalke:
    review ACK 32fa49a184 🍢

Tree-SHA512: 7752193117669b800889226185d49d164395697853828f8acb568f07651789bc5b2cddc45555957450353886e46b9a1e13c77a5e730a14c6ee621fabc8dc3d10
2021-08-04 19:02:04 +02:00
Vasil Dimov
87651795d8
fuzz: check that ser+unser produces the same AddrMan 2021-08-04 18:22:08 +02:00
MarcoFalke
2b06af1747
Merge bitcoin/bitcoin#22603: doc: Add release notes for 22570 (ignore banlist.dat)
fa2c868886 doc: Add release notes for 22570 (ignore banlist.dat) (MarcoFalke)

Pull request description:

  Follow-up to #22570

ACKs for top commit:
  tryphe:
    ACK fa2c868886
  vasild:
    ACK fa2c868886
  Zero-1729:
    ACK fa2c868886

Tree-SHA512: 58eef340b6211bcdecf3826ac145afd476c397f110daa6783521c0c1e1be1ffbd2d24ad20c77921abbe5cdb8e644d3cd64e14e2819746cf0e5123fb7cc445d63
2021-08-04 17:50:09 +02:00
MarcoFalke
513e1071a1
Merge bitcoin/bitcoin#22616: p2p, rpc: address relay fixups
5e33f762d4 p2p, rpc: address relay fixups (Jon Atack)

Pull request description:

  Following review of new changes merged today, move a use of `statestats` in getpeerinfo to within the section guarded by `if (fStateStats)`, e.g. `PeerManagerImpl::GetNodeStateStats` true, and pass an in-param by reference to const.

ACKs for top commit:
  amitiuttarwar:
    ACK 5e33f762d4
  jnewbery:
    ACK 5e33f762d4

Tree-SHA512: b42f33c615b14079e2c4e6060209de8707d71b351dd1e11e04a2a6fc12d15747d0c5d9b24850217080fd1ef92e63f96d6925c4badf280b781edd696c349be7d6
2021-08-04 17:17:55 +02:00
MarcoFalke
5b2d8661c9
Merge bitcoin/bitcoin#22577: Close minor startup race between main and scheduler threads
703b1e612a Close minor startup race between main and scheduler threads (Larry Ruane)

Pull request description:

  This is a low-priority bug fix. The scheduler thread runs `CheckForStaleTipAndEvictPeers()` every 45 seconds (EXTRA_PEER_CHECK_INTERVAL). If its first run happens before the active chain is set up (`CChain::SetTip()`), `bitcoind` will assert:
  ```
  (...)
  2021-07-28T22:16:49Z init message: Loading block index…
  bitcoind: validation.cpp:4968: CChainState& ChainstateManager::ActiveChainstate() const: Assertion `m_active_chainstate' failed.
  Aborted (core dumped)
  ```
  I ran into this while using the debugger to investigate an unrelated problem. Single-stepping through threads with a debugger can cause the relative thread execution timing to be very different than usual. I don't think any automated tests are needed for this PR. I'll give reproduction steps in the next PR comment.

ACKs for top commit:
  MarcoFalke:
    cr ACK 703b1e612a
  tryphe:
    tested ACK 703b1e612a
  0xB10C:
    ACK 703b1e612a
  glozow:
    code review ACK 703b1e612a - it makes sense to me to start peerman's background tasks here, after `chainstate->LoadChainTip()` and `node.connman->Start()` have been called.

Tree-SHA512: 9316ad768cba3b171f62e2eb400e3790af66c47d1886d7965edb38d9710fc8c8f8e4fb38232811c9346732ce311d39f740c5c2aaf5f6ca390ddc48c51a8d633b
2021-08-04 16:37:12 +02:00
fanquake
32fa49a184
make ParseOutputType return a std::optional<OutputType> 2021-08-04 19:20:32 +08:00
fanquake
3308c61091
Merge bitcoin/bitcoin#22576: doc: Update high-level addrman description
036d7eadf5 doc: Correct description of CAddrMan::Create() (Amiti Uttarwar)
318176aff1 doc: Update high-level addrman description (Martin Zumsande)

Pull request description:

  The high-level description of `addrman` has outdated information with respect to the eviction behavior, both for the New and Tried tables (at least since #5941) - this has confused me in the past.

  This PR corrects this and also adds basic info about the bucket size and position.

ACKs for top commit:
  amitiuttarwar:
    reACK 036d7eadf5
  jnewbery:
    ACK 036d7eadf5

Tree-SHA512: 3f0635d765f5e580a1fae31187742a833cef66ef2286d40eeb28f2253521260038e16e5f1a65741464a2ddfdbeb5c0f1bc38bf73841e600639033d59c3c534e4
2021-08-04 10:53:08 +08:00
Hennadii Stepanov
d54d949598
qt: Fix regression in "Encrypt Wallet" menu item
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.
2021-08-03 22:21:34 +03:00
fanquake
eaf09bda4a
Merge bitcoin/bitcoin#22609: [GetTransaction] remove unneeded cs_main lock acquire
4a1b2a7ba7 [GetTransaction] remove unneeded `cs_main` lock acquire (Sebastian Falbesoner)

Pull request description:

  This PR is a follow-up to #22383. For reading from the mempool, only `mempool.cs` needs to be locked (see [suggestion by MarcoFalke](https://github.com/bitcoin/bitcoin/pull/22383#discussion_r675069128)):

  b620b2d58a/src/txmempool.h (L554-L558)

  `CTxMemPool::get()` acquires this lock:

  b620b2d58a/src/txmempool.cpp (L822-L829)

   so we don't need to acquire any lock ourselves in `GetTransaction()`, as the other functions called in the remaining parts also don't need to have `cs_main` locked.

ACKs for top commit:
  tryphe:
    Concept ACK. tested 4a1b2a7ba7 but not extensively.
  jnewbery:
    Code review ACK 4a1b2a7ba7

Tree-SHA512: 60e869f72e65cf72cb144be1900ea7f3d87c12f322756994f6a3ed8cd975230b36c7c90c34b60bbf41f9186f4add36decaac1d4f0d0749fb5451b3938a8aa78c
2021-08-03 20:07:26 +08:00
MarcoFalke
31a481be29
Merge bitcoin/bitcoin#22597: consensus/params: simplify ValidDeployment check to avoid gcc warning
059171009b consensus/params: simplify ValidDeployment check to avoid gcc warning (Anthony Towns)

Pull request description:

  Simplifies the ValidDeployment check to only check the upperbound, relying on the lower bound to be trivially true due to enum values starting at the minimum value of the underlying type (which is checked at compile time in deploymentstatus.cpp). Avoids a "comparison always true" warning in some versions of gcc.

  Fixes #22587

ACKs for top commit:
  MarcoFalke:
    review ACK 059171009b
  tryphe:
    retested ACK 059171009b

Tree-SHA512: e53b5d478b46d35ec476d004e3c92803afb874c138dd6ef3848861f4281cc113fe88921bc4ac74fd4decbf318ed776d3f816c3a1185f99dc36a5cfecfff51f7c
2021-08-03 13:54:27 +02:00
Jon Atack
5e33f762d4
p2p, rpc: address relay fixups 2021-08-03 13:09:19 +02:00
MarcoFalke
87257d860e
Merge bitcoin/bitcoin#22532: test : improve mempool_accept_wtxid.py
91b05974fc Improve mempool_accept_wtxid.py (naiza)

Pull request description:

  Follow-up to #22253 adding changes suggested in [#22253 (review)](https://github.com/bitcoin/bitcoin/pull/22253#discussion_r666933370)

ACKs for top commit:
  glozow:
    utACK 91b05974fc

Tree-SHA512: 383064138a5b2160d769c9df370470fd585c91682083013a6fa15e14448a4b481bc09b3a0ed6e75554db2c378df6b2263c65f209f973c9e9d577e15814a4be1d
2021-08-03 11:04:50 +02:00
MarcoFalke
ad0fc453cc
Merge bitcoin/bitcoin#16333: test: Set BIP34Height = 2 for regtest
222290f543 test: Set BIP34Height = 2 for regtest (MarcoFalke)
fac90c55be test: Create all blocks with version 4 or higher (MarcoFalke)

Pull request description:

  BIP34 is active on the current tip of mainnet, so all miners must obey it. It would be nice if it also was active in fresh regtest instances from the earliest time possible.

  I changed the BIP34 height to `2`, so that the block at height=1 may be used to mine a duplicate coinbase. (Needed to test mainnet behaviour)

  This pull is done in two commits:

  *  test: Create all blocks with version 4 or higher:
     Now that BIP34 is activated earlier, we need to create blocks with a higher version number. Just bump it to 4 instead of 2 to avoid having to bump it again later.

  *  test: Set BIP34Height = 2 for regtest:
     This fixes the BIP34 implementation in the tests (to match the one of the Core codebase) and updates the tests where needed

ACKs for top commit:
  ajtowns:
    ACK 222290f543
  jonatack:
    ACK 222290f543 tested and reviewed rebased to current master 5e213822f8
  theStack:
    Tested ACK 222290f543

Tree-SHA512: d69c637a62a64b8e87de8c7f0b305823d8f4d115c1852514b923625dbbcf9a4854b5bb3771ff41702ebf47c4c182a4442c6d7c0b9f282c95a34b83e56a73939b
2021-08-03 10:10:43 +02:00
fanquake
5cf28d5203
Merge bitcoin/bitcoin#22496: addrman: Remove addrman hotfixes
65332b1178 [addrman] Remove RemoveInvalid() (John Newbery)

Pull request description:

  PRs #22179 and #22112 (EDIT: later reverted in #22497) added hotfix code to addrman to remove invalid addresses and mutate the ports of I2P entries after entering into addrman. Those hotfixes included at least two addrman data corruption bugs:

  - #22467 (Assertion `nRndPos1 < vRandom.size() && nRndPos2 < vRandom.size()' failed)
  - #22470 (Changing I2P ports in addrman may wronly skip some entries from "new" buckets)

  Hotfixing addrman is inherently dangerous. There are many members that have implicit assumptions on each others' state, and mutating those directly can lead to violating addrman's internal invariants.

  Instead of trying to hotfix addrman, just don't insert any invalid addresses. For now, those are addresses which fail `CNetAddr::IsValid()`.

ACKs for top commit:
  sipa:
    utACK 65332b1178. I tried to reason through scenarios that could introduce inconsistencies with this code, but can't find any.
  fanquake:
    ACK 65332b1178 - Skipping the addition of invalid addresses (this code was initially added for Tor addrs) rather than adding all the invalids then removing them all when finishing unserializing seems like an improvement. Especially if it can be achieved with less code.

Tree-SHA512: 023113764cb475572f15da7bf9824b62b79e10a7e359af2eee59017df354348d2aeed88de0fd4ad7a9f89a0dad10827f99d70af6f1cb20abb0eca2714689c8d7
2021-08-03 15:40:32 +08:00
fanquake
71797beec5
Merge bitcoin/bitcoin#22448: Update libsecp256k1 subtree to latest upstream
c020cbaa5c Squashed 'src/secp256k1/' changes from efad3506a8..be8d9c262f (Pieter Wuille)

Pull request description:

  This updates our src/secp256k1 subtree to the lastest upstream master. Notable changes:
  * New schnorrsig API (https://github.com/bitcoin-core/secp256k1/pull/844), which adds support for variable-length messages (not used in BIP341/342 transaction signing, so not relevant for us, but it changes the API, and makes some other simplifications). Some of our call sites had to be adapted.
  * Don't use asm optimizations for `gen_context` (https://github.com/bitcoin-core/secp256k1/pull/965). This fixes #22441.
  * Various testing/CI improvements

ACKs for top commit:
  hebasto:
    ACK e4ffb44716
  jonatack:
    Light ACK e4ffb44716 debug built (debian clang 13.0), ran bitcoind node/tests/git-subtree-check.sh, lightly reviewed the diff and API changes
  fanquake:
    ACK e4ffb44716

Tree-SHA512: 89a5c3019ec010d578e84bcef756d2c679420c5c768bcdece673405c4e10955179c5a1339aafc68b8b74b1e3912e147bf2f392f44f15af73791d93f6537960b3
2021-08-03 13:15:22 +08:00
fanquake
10fbb37268
Merge bitcoin/bitcoin#22098: [test, init] DNS seed querying logic
82b6f89819 [style] Small style improvements to DNS parameters (Amiti Uttarwar)
4c89e24f64 [test] Test the delay before querying DNS seeds (Amiti Uttarwar)
6395c8ed56 [test] Test the interactions between -forcednsseed and -dnsseed (Amiti Uttarwar)
6f6b7df6bd [init] Disallow starting up with conflicting paramters for -dnsseed and -forcednsseed (Amiti Uttarwar)
26d0ffe4f2 [test] Test -forcednsseed causes querying DNS seeds (Amiti Uttarwar)
35851450a9 [test] Test the interactions between -connect and -dnsseed (Amiti Uttarwar)
75c05af361 [test] Test logic to query DNS seeds with block-relay-only connections (Amiti Uttarwar)
9c08719778 [test] Introduce test logic to query DNS seeds (Amiti Uttarwar)

Pull request description:

  This PR adds a DNS seed to the regtest chain params to enable testing the DNS seed querying logic of `CConnman::ThreadDNSAddressSeed` and relevant startup parameters. Adds coverage for the changes in #22013 (and then some).

  The main behavioral change to bitcoind is that this PR disallows starting up with conflicting parameters for `-dnsseed` and `-forcednsseed`.

  The tests include:
  * parameter interactions of different combinations of `-connect`, `-dnsseed` and `-forcednsseed`
  * the delay before querying DNS seeds depending on how many addresses are in the addrman
  * the behavior of `-forcednsseed`
  * skipping DNS querying if we have outbound full relay connections & not block-relay-only connections

  Huge props to mzumsande for identifying the timing technique for testing successful connections before running `ThreadDNSAddressSeed` 🙌🏽

ACKs for top commit:
  mzumsande:
    ACK 82b6f89819
  jnewbery:
    reACK 82b6f89819

Tree-SHA512: 9f0c29bfbf99426727e79c0a25606ae09deab91a92e3c5cee7f84c3ca7503a8ac9ab85a85c51841d40b164ef8c991326070f0b2f41d075fb7985df26f6e95d6d
2021-08-03 11:21:15 +08:00