Skip to content

Merge #17119,1683,17102,17134,17691,17992,18170,18382,16975,18472, 18486,17633,7833 #4744

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
PastaPastaPasta merged 10 commits into from
Apr 3, 2022
Merged

Merge #17119,1683,17102,17134,17691,17992,18170,18382,16975,18472, 18486,17633,7833 #4744

PastaPastaPasta merged 10 commits into from
Apr 3, 2022

Conversation

@vijaydasmp
Copy link

@vijaydasmp vijaydasmp commented Mar 29, 2022

85016e5 [rpc] Fix broken bitcoin-cli examples (Andrew Toth)

Pull request description:

This fixes the bitcoin-cli examples for combinerawtransaction, combinepsbt and testmempoolaccept. They currently return Error parsing JSON.

ACKs for top commit:
laanwj:
ACK 85016e5

Tree-SHA512: b561f68f7a188dc91dab1ceb98da3ac3e232143ab2b906c90f95c6b74b584599d0f3b51f067cdd3b1153931f95b3dc385e453b1a0dde86f9cb549b94560f219d

@vijaydasmp vijaydasmp changed the title Merge #17119: doc: Fix broken bitcoin-cli examples Merge #17119,1683,17102,17134,17691,17992,18170,18382,18486,17833 Mar 31, 2022
@vijaydasmp vijaydasmp marked this pull request as ready for review March 31, 2022 03:11
@vijaydasmp
Copy link
Author

vijaydasmp commented Mar 31, 2022

Hello @UdjinM6 @PastaPastaPasta, please review

UdjinM6
UdjinM6 requested changes Mar 31, 2022
View reviewed changes
doc/files.md Outdated
Comment on lines 25 to 27
Copy link

@UdjinM6 UdjinM6 Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

16983:

Suggested change
Linux | `$HOME/.dash/`
macOS | `$HOME/Library/Application Support/Dash/`
Windows | `%APPDATA%\Dash\` <sup>[\[1\]](#note1)</sup>
Linux | `$HOME/.dashcore/`
macOS | `$HOME/Library/Application Support/DashCore/`
Windows | `%APPDATA%\DashCore\` <sup>[\[1\]](#note1)</sup>

doc/files.md Outdated
Copy link

@UdjinM6 UdjinM6 Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

16983: v3

contrib/devtools/circular-dependencies.py Outdated
Copy link

@UdjinM6 UdjinM6 Mar 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

17691: do not dashify copyrights in non-dash-specific files (and in files we made no significant changes to), same below

@vijaydasmp vijaydasmp changed the title Merge #17119,1683,17102,17134,17691,17992,18170,18382,18486,17833 Merge #17119,1683,17102,17134,17691,17992,18170,18382,16975,1847218486,17833 Apr 1, 2022
@vijaydasmp vijaydasmp changed the title Merge #17119,1683,17102,17134,17691,17992,18170,18382,16975,1847218486,17833 Merge #17119,1683,17102,17134,17691,17992,18170,18382,16975,18472, 18486,17833 Apr 1, 2022
@vijaydasmp vijaydasmp changed the title Merge #17119,1683,17102,17134,17691,17992,18170,18382,16975,18472, 18486,17833 Merge #17119,1683,17102,17134,17691,17992,18170,18382,16975,18472, 18486,17633,7833 Apr 1, 2022
laanwj and others added 5 commits April 2, 2022 09:19
@laanwj @vijaydasmp
Merge bitcoin#17119: doc: Fix broken bitcoin-cli examples
609281c 
85016e5 [rpc] Fix broken bitcoin-cli examples (Andrew Toth)

Pull request description:

  This fixes the `bitcoin-cli` examples for `combinerawtransaction`, `combinepsbt` and `testmempoolaccept`. They currently return `Error parsing JSON`.

ACKs for top commit:
  laanwj:
    ACK 85016e5

Tree-SHA512: b561f68f7a188dc91dab1ceb98da3ac3e232143ab2b906c90f95c6b74b584599d0f3b51f067cdd3b1153931f95b3dc385e453b1a0dde86f9cb549b94560f219d
@vijaydasmp
Merge bitcoin#16983: doc: Add detailed info about Dash Core files
f8ce569 
86b9f92 doc: Add detailed info about Bitcoin Core files (Hennadii Stepanov)

Pull request description:

  This PR:
  - provides detailed info about the Bitcoin Core files;
  - does not mention temporary files, e.g., `mempool.dat.new` and `peers.????`

ACKs for top commit:
  ch4ot1c:
    ACK 86b9f92
  laanwj:
    ACK 86b9f92
  MarcoFalke:
    ACK 86b9f92

Tree-SHA512: 9352119b08e3f6aaab4ce3797afc6533f90852e461957acb2bc73962fd4881403fabeaa5a371bd1218309f36f9b0f90fb147b80698e2e30a016634a62a160a15
@fanquake @vijaydasmp
Merge bitcoin#17102: doc: Add missing indexes/blockfilter/basic to do…
f4ca952 
…c/files.md

fa191c0 doc: Add missing indexes/blockfilter/basic/ to doc/files.md (MarcoFalke)

Pull request description:

ACKs for top commit:
  hebasto:
    ACK fa191c0
  fanquake:
    ACK fa191c0

Tree-SHA512: ea30776224e4a26bc6f60c9c65361f0b9d7b2b4f844c6d2aefd108f852dd7d813963df631a8ed20daea5acae7a23d0b405d12a162f90a6f61a3db236f5beed45
@fanquake @vijaydasmp
Merge bitcoin#17134: doc: Add switch on enum example to developer notes
db662cf 
c8961c7 doc: Add switch on enum example (Hennadii Stepanov)
11e3d5e util: Add AllowShortCaseLabelsOnASingleLine option (Hennadii Stepanov)

Pull request description:

  This PR documents a recurring issue:
  - bitcoin#15938
  - bitcoin#17105

ACKs for top commit:
  laanwj:
    Seems like good advice to me. ACK c8961c7
  practicalswift:
    ACK c8961c7
  promag:
    ACK c8961c7, no excuse now, thanks!

Tree-SHA512: 530da5117094ed1bfaa6e447089521bd2c86b0742758dbacec4e4f934dc07b0e24f15a1448c4d58e49905e8fd3797d87bcae5669a346d33ed4c2878a04891699
@vijaydasmp
Merge bitcoin#17691: doc: Add missed copyright headers
6270a09 
fac86ac scripted-diff: Add missed copyright headers (Hennadii Stepanov)
6fde9d5 script: Update EXLUDE list in copyright_header.py (Hennadii Stepanov)
1998152 script: Add empty line after C++ copyright (Hennadii Stepanov)
071f2fc script: Add ability to insert copyright to *.sh (Hennadii Stepanov)

Pull request description:

  This PR improves `contrib/devtools/copyright_header.py` script and adds copyright headers to the files in `src` and `test` directories with two exceptions:
  - [`src/reverse_iterator.h`](https://github.com/bitcoin/bitcoin/blob/master/src/reverse_iterator.h) (added to exceptions)
  - [`src/test/fuzz/FuzzedDataProvider.h`](https://github.com/bitcoin/bitcoin/blob/master/src/test/fuzz/FuzzedDataProvider.h) (added to exceptions)

  On master 5622d8f:
  ```
  $ ./contrib/devtools/copyright_header.py report . | grep zero
    25 with zero copyrights
  ```

  With this PR:
  ```
  $ ./contrib/devtools/copyright_header.py report . | grep zero
     2 with zero copyrights
  ```

  ~I am uncertain about our copyright policy with `build_msvc` and `contrib` directories content, so they are out of scope of this PR.~

ACKs for top commit:
  MarcoFalke:
    ACK fac86ac

Tree-SHA512: d7832c4a7a1a3b7806119775b40ec35d7982f49ff0e6199b8cee4c0e0a36e68d51728b6ee9924b1c161df4bc6105bd93391b79d42914357fa522f499cb113fa8
@vijaydasmp vijaydasmp force-pushed the bp2003 branch 2 times, most recently from 6a96f20 to 2293dfb Compare April 2, 2022 16:40
laanwj and others added 4 commits April 3, 2022 08:35
@laanwj @vijaydasmp
Merge bitcoin#16975: test: Show debug log on unit test failure
92798c6 
fa37e0a test: Show debug log on unit test failure (MarcoFalke)

Pull request description:

  Often, it is hard to debug unit test failures without the debug log. Especially when the failure happens remotely (e.g. on a ci system).

  Fix that by printing the log on failure.

ACKs for top commit:
  jamesob:
    ACK fa37e0a ([`jamesob/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u`](https://github.com/jamesob/bitcoin/tree/ackr/16975.1.MarcoFalke.test_show_debug_log_on_u))

Tree-SHA512: 2ca4150c4ae3d4ad47e03b5e5e70da2baffec928ddef1fdf53a3ebc061f14aee249205387cb1b12ef6d4eb55711ef0080c0b41d9d18000b5da124ca80299793b
@vijaydasmp
Merge bitcoin#18472: test: Remove unsafe BOOST_TEST_MESSAGE
639386a 
fa3cc0b test: Remove unsafe BOOST_TEST_MESSAGE (MarcoFalke)

Pull request description:

  Fixes bitcoin#17987

  Can be tested with

  ```
  ./src/test/test_bitcoin -l test_suite  -- DEBUG_LOG_OUT

ACKs for top commit:
  fjahr:
    tested ACK fa3cc0b
  mzumsande:
    Tested ACK fa3cc0b

Tree-SHA512: f63b110d77882cd7c0d7574ff6c9c948db8febb3400ecdac45164746b587b0fa223463041801271b3959267ddc1d9a4a67ba76939e242e7dd2f92a2834a400a0
@vijaydasmp
Merge bitcoin#18486: doc: Explain new test logging
cacc5c4 
7777703 doc: Explain new test logging (MarcoFalke)

Pull request description:

  Explain logging added in bitcoin#18472 and bitcoin#16975

ACKs for top commit:
  jonatack:
    ACK 7777703

Tree-SHA512: 3a0aa7bab32a6753d8894d29cf82604b044b23e512102dd275b717eefda3c2212dbf43ea7e9155267350dd9f3bc5badba2eb660152db3efeab30a04f52126c95
@vijaydasmp
Merge bitcoin#17633: tests: Add option --valgrind to run the function…
77f106e 
…al tests under Valgrind

5db506b tests: Add option --valgrind to run nodes under valgrind in the functional tests (practicalswift)

Pull request description:

  What is better than fixing bugs? Fixing entire bug classes of course! :)

  Add option `--valgrind` to run the functional tests under Valgrind.

  Regular functional testing under Valgrind would have caught many of the uninitialized reads we've seen historically.

  Let's kill this bug class once and for all: let's never use an uninitialized value ever again. Or at least not one that would be triggered by running the functional tests! :)

  My hope is that this addition will make it super-easy to run the functional tests under Valgrind and thus increase the probability of people making use of it :)

  Hopefully `test/functional/test_runner.py --valgrind` will become a natural part of the pre-release QA process.

  **Usage:**

  ```
  $ test/functional/test_runner.py --help
  …
    --valgrind            run nodes under the valgrind memory error detector:
                          expect at least a ~10x slowdown, valgrind 3.14 or
                          later required
  ```

  **Live demo:**

  First, let's re-introduce a memory bug by reverting the recent P2P uninitialized read bug fix from PR bitcoin#17624 ("net: Fix an uninitialized read in ProcessMessage(…, "tx", …) when receiving a transaction we already have").

  ```
  $ git diff
  diff --git a/src/consensus/validation.h b/src/consensus/validation.h
  index 3401eb6..940adea33 100644
  --- a/src/consensus/validation.h
  +++ b/src/consensus/validation.h
  @@ -114,7 +114,7 @@ inline ValidationState::~ValidationState() {};

   class TxValidationState : public ValidationState {
   private:
  -    TxValidationResult m_result = TxValidationResult::TX_RESULT_UNSET;
  +    TxValidationResult m_result;
   public:
       bool Invalid(TxValidationResult result,
                    const std::string &reject_reason="",
  ```

  Second, let's test as normal without Valgrind:

  ```
  $ test/functional/p2p_segwit.py -l INFO
  2019-11-28T09:30:42.810000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test__fc8q3qo
  …
  2019-11-28T09:31:57.187000Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
  …
  2019-11-28T09:32:08.265000Z TestFramework (INFO): Tests successful
  ```

  Third, let's test with `--valgrind` and see if the test fail (as we expect) when the unitialized value is used:

  ```
  $ test/functional/p2p_segwit.py -l INFO --valgrind
  2019-11-28T09:32:33.018000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_gtjecx2l
  …
  2019-11-28T09:40:36.702000Z TestFramework (INFO): Subtest: test_non_standard_witness_blinding (Segwit active = True)
  2019-11-28T09:40:37.813000Z TestFramework (ERROR): Assertion failed
  ConnectionRefusedError: [Errno 111] Connection refused
  ```

ACKs for top commit:
  MarcoFalke:
    ACK 5db506b
  jonatack:
    ACK 5db506b

Tree-SHA512: 2eaecacf4da166febad88b2a8ee6d7ac2bcd38d4c1892ca39516b6343e8f8c8814edf5eaf14c90f11a069a0389d24f0713076112ac284de987e72fc5f6cc3795
@vijaydasmp
Merge bitcoin#17833: doc: Added running functional tests in valgrind
f80a99c 
155a11f doc: Added running functional tests in valgrind (Elichai Turkel)

Pull request description:

  Technically the notes only show an "example" of how to run valgrind with the suppression file,
  but now that bitcoin#17633 is merged then maybe this can encourage more people to run also the functional tests in valgrind

Top commit has no ACKs.

Tree-SHA512: b8417249b720d0ed5e10b732648f2e07e8889bfc7aa7e94192d1c049b4b7837971678d30c535f273c227848f1290cf11e14369fd6c1924b734f2e47e2af41401
UdjinM6
UdjinM6 approved these changes Apr 3, 2022
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@UdjinM6 UdjinM6 requested a review from PastaPastaPasta April 3, 2022 09:44
PastaPastaPasta
PastaPastaPasta approved these changes Apr 3, 2022
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK for merging via merge commit

@PastaPastaPasta PastaPastaPasta merged commit e84bf45 into dashpay:develop Apr 3, 2022
@UdjinM6 UdjinM6 added this to the 18 milestone Apr 3, 2022
@Liquid369 Liquid369 mentioned this pull request Oct 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

18

Development

Successfully merging this pull request may close these issues.

5 participants

@vijaydasmp @UdjinM6 @PastaPastaPasta @laanwj @fanquake