Skip to content

Merge bitcoin PRs: #14683, #15841, #14984 and #15943 #4514

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
UdjinM6 merged 4 commits into from
Oct 13, 2021
Merged

Merge bitcoin PRs: #14683, #15841, #14984 and #15943 #4514

UdjinM6 merged 4 commits into from
Oct 13, 2021

Conversation

@dzutto
Copy link

@dzutto dzutto commented Oct 12, 2021

No description provided.

MarcoFalke added 4 commits October 12, 2021 15:56
@dzutto
Merge bitcoin#14683: tests: better combine_logs.py behavior
772dad9 
4aabadb tests: have combine_logs default to most recent test dir (James O'Beirne)

Pull request description:

  Have `combine_logs.py` default to the most recent test directory if no argument is provided. This allows you to avoid an annoying copy-paste when iterating on a failing test, since you can do something like
  ```sh
  alias testlogs='./test/functional/combine_logs.py -c | less'

  ./test/functional/some_test.py  # fails
  testlogs
  ```

Tree-SHA512: 919642ab09c314888a23c9491963b35b9da87e60deb740d1d5e816444aa9bdda5e519dc8ca131669f2d563167ef5f5abb14e22f20f47bf8362915ed578181846
@dzutto
Merge bitcoin#15841: [test] combine_logs: append node stderr and stdo…
0d58cda 
…ut if it exists

fa90a89 [test] combine_logs: append node stderr and stdout if it exists (MarcoFalke)

Pull request description:

  See issue:

  * tests: bitcoind stdout and error should be passed to the logger bitcoin#13519

ACKs for commit fa90a8:
  laanwj:
    utACK fa90a89

Tree-SHA512: 39c4596e2e133c9011ab01bc4dc24e884d0a8cce7a67d3765f17c288d3ffbd438e1ff6016d0f817a981b27fce17fa77a1ff56787ddb1ea55123ce9ecffb44c08
@dzutto
Merge bitcoin#14984: rpc: Speedup getrawmempool when verbose=true
fec569b 
2d5cf4c rpc: Speedup getrawmempool when verbose=true (João Barbosa)

Pull request description:

  Instead of calling `pushKV(hash, info)`, which incurs in duplicate key checks, call `__pushKV` which (currently) doesn't.

  Improves RPC `getrawmempool` and REST `/rest/mempool/contents.json`.

  Fixes bitcoin#14765.

ACKs for commit 2d5cf4:

Tree-SHA512: c3e91371bb41f39e79dcef820815e1dc27fb689ca3c4bf3a00467d2215b3baecd44d9792f7a481577a5b7ae1fc6cbaa07b1cd62123b845082eba65b35c2b3ca5
@dzutto
Merge bitcoin#15943: tests: Fail if RPC has been added without tests
652a36b 
fad0ce5 tests: Fail if RPC has been added without tests (MarcoFalke)

Pull request description:

  Need to be run with --coverage

ACKs for commit fad0ce:
  ryanofsky:
    utACK fad0ce5. New comment in travis.yml is the only change since last review.

Tree-SHA512: b53632dfe9865ec06991bfcba2fd67238bebbb866b355f09624eaf233257b2bca902caac6c24abb358b2f4c1c43f28ca75e30982765911e1a117102df65276d9
@dzutto
Copy link
Author

dzutto commented Oct 12, 2021

A question about bitcoin#15943 - right now it doesn't exactly fail, rather, it prints out the list of RPCs that aren't covered with tests:

Uncovered RPC commands:
  - debug
  - getblockheaders
  - getgovernanceinfo
  - getmerkleblocks
  - getpoolinfo
  - getspecialtxes
  - getsuperblockbudget
  - voteraw

These are mostly Dash-specific RPCs.
Do we want to check this in as is and add missing tests later or implement them right away and merge along?

PastaPastaPasta
PastaPastaPasta previously approved these changes Oct 13, 2021
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, one nit

test/functional/test_framework/test_framework.py
TEST_EXIT_SKIPPED = 77

GENESISTIME = 1417713337
TMPDIR_PREFIX = "dash_func_test_"
Copy link
Member

@PastaPastaPasta PastaPastaPasta Oct 13, 2021

Choose a reason for hiding this comment

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

14683 nit, should be a new-line after this

Copy link
Author

@dzutto dzutto Oct 13, 2021

Choose a reason for hiding this comment

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

There is already a newline here, and the original chunk in bitcoin#14683 doesn't have any new line after TMPDIR_PREFIX: https://github.com/bitcoin/bitcoin/pull/14683/files#diff-defba63b5060365ddb5ea30737e06bae40f15a743f437b6fff18dc15e9acc233R16-R22

Do you want me to add one between GENESISTIME and TMPDIR_PREFIX?

Copy link
Member

@PastaPastaPasta PastaPastaPasta Oct 13, 2021

Choose a reason for hiding this comment

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

No, there should be ANOTHER newline after TMPDIR_PREFIX :D If you look at the bitcoin PR, there is two new lines after the TMPDIR_PREFIX

Copy link
Author

@dzutto dzutto Oct 13, 2021

Choose a reason for hiding this comment

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

There is just one as far as I can tell... At line 23:
image

Not like I mind adding another one though - will do :)

Copy link
Author

@dzutto dzutto Oct 13, 2021

Choose a reason for hiding this comment

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

Oops, didn't realize the PR has already been merged in overnight. Anyways, hope that's not a big deal :-D

@PastaPastaPasta PastaPastaPasta added this to the 18 milestone Oct 13, 2021
@UdjinM6 UdjinM6 mentioned this pull request Oct 13, 2021
Closed
PastaPastaPasta
PastaPastaPasta approved these changes Oct 13, 2021
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.

re-utACK

UdjinM6
UdjinM6 approved these changes Oct 13, 2021
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 merged commit 4ac54be into dashpay:develop Oct 13, 2021
@dzutto dzutto deleted the merge_14683_15841_14984_15943 branch October 13, 2021 15:27
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

Core Dev Backlog
Status: Done

Milestone

18

Development

Successfully merging this pull request may close these issues.

3 participants

@dzutto @UdjinM6 @PastaPastaPasta