Skip to content

Conversation

@Munkybooty
Copy link

No description provided.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is correct, since we've never made it such that bip70 is disabled by default. Instead it should be disable_bip70=no etc

Copy link
Member

Choose a reason for hiding this comment

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

16845: I think this should wait until we backport 16796?

Copy link
Member

Choose a reason for hiding this comment

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

16512: this doesn't do anything

@github-actions
Copy link

This pull request has conflicts, please rebase.

@github-actions
Copy link

This pull request has conflicts, please rebase.

@Munkybooty Munkybooty force-pushed the backports-0.19-pr9 branch 4 times, most recently from 2821799 to 451c7ee Compare December 27, 2021 03:18
Copy link
Member

Choose a reason for hiding this comment

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

you should simply drop 16898, we already merged it (and did this change, some other PR must have reverted)

Copy link
Member

Choose a reason for hiding this comment

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

16512 is either incomplete or dnm

Copy link
Author

Choose a reason for hiding this comment

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

It seems like all the additions to joinpsbts was not added which could explain why including random did nothing

Copy link
Member

Choose a reason for hiding this comment

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

14696 was already merged, should be dropped (your change is duplicative)

Copy link
Member

Choose a reason for hiding this comment

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

imo, should merge bitcoin#14457 before 16885

Copy link
Member

Choose a reason for hiding this comment

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

16918 was already merged, drop

@github-actions
Copy link

This pull request has conflicts, please rebase.

@github-actions
Copy link

This Pull Request may conflict if the Pull Requests below are merged first.

#4650
conflictable files: src/validation.cpp

@Munkybooty Munkybooty force-pushed the backports-0.19-pr9 branch 2 times, most recently from 6bb38fa to 1afbbc2 Compare January 1, 2022 00:41
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.

pls see 912294b9a64dc636782112c95ee4f8ae162e3576

@Munkybooty
Copy link
Author

I had fixed feature_block.py but it appears that p2p_invalid_tx.py fails now. I'm not sure whats caused this new error.

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.

pls see below and #4668 (will require rebase to fix tests)

@github-actions
Copy link

This pull request has conflicts, please rebase.

fanquake and others added 7 commits January 20, 2022 13:09
…tcoin-wallet's help

b6233a4 bitcoin-wallet: Add a missing closing parenthesis in the help (darosior)

Pull request description:

ACKs for top commit:
  kristapsk:
    utACK b6233a4
  fanquake:
    ACK b6233a4

Tree-SHA512: acf18633fdca4bd73838fcaa0ebe4121dd0b5308daa77c4458ec4c98a9e8aa6d9d6580a48c884147438af14e670b0606c1e76f72d1d7efd221c4da419061beed
…onf, and Info.plist.in

6aab764 doc: Fix whitespace errs in .md files, bitcoin.conf, Info.plist.in, and find_bdb48.m4 (Jon Layton)

Pull request description:

  Although there is an existing `test/lint/lint-whitespace.sh` linter, it only prevents new errors from being introduced. This commit removes all existing whitespace errors from Core markdown files (skips `src/crypto/ctaes/`, `leveldb/`, and `doc/release-notes/`), `bitcoin.conf`, and `Info.plist.in`.

  Further formatting could be done on the markdown documents, but seeing as there several coexisting styles that break a few `markdownlint` rules, a first step would be to define and add a linter to Travis. For now, the small fix is made.

ACKs for top commit:
  fanquake:
    ACK 6aab764 - Thanks for following up. Hopefully we now never have to deal with whitespace again.

Tree-SHA512: 810cc31ae4364b2dedf85783e67315d7b4e11589e4b32c599606e1b1ba8de0663bcae9ddb1bd8c9762a3636a2d65bdcd64ec22d2e90943f374a0c9574b77ca23
c0b5d97 Test that joinpsbts randomly shuffles the inputs (Andrew Chow)
6f405a1 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 bitcoin@c0b5d97
  jonatack:
    ACK c0b5d97 modulo suggestions for later.

Tree-SHA512: 14a0b7aae07d92e6d2c76a3a3b228b481e1964cb7d34f97515bdda18e2ea05a9f97c5a22affc143b86ae8b95c3cb239849fb54219d65512bc2112264dca915c8
…high_minversion

2222c96 test: Add notes on how to generate data/wallets/high_minversion (MarcoFalke)

Pull request description:

  I forgot to do this in bitcoin#16796

ACKs for top commit:
  ryanofsky:
    ACK 2222c96

Tree-SHA512: 5f24ffa641b97eac4febad42ade7228b14fa72335c918a10880c5dec86a3ecc3075a31526f275188e07fea95b8e2c6320c64f716099f604b00e13d5366fcee37
…onal tests

59e3877 test: add invalid tx templates for use in functional tests (James O'Beirne)

Pull request description:

  This change adds a list of `CTransaction`-generating templates which each correspond to a specific type of invalid transaction. We then use this list to test for a wider variety of invalid tx types in `p2p_invalid_tx.py` and `feature_block.py`.

  Consolidating all invalid tx types will allow us to more easily cover all tx reject cases from a variety of tests without repeating ourselves. Validation logic doesn't differ much between mempool and block acceptance, but there *is* a difference and we should be sure we're testing both comprehensively.

  Right now, I've only added templates covering the tx reject types listed below but if this approach seems worthwhile I will expand the list to be fully comprehensive.
  ```
  bad-txns-in-belowout
  bad-txns-inputs-duplicate
  bad-txns-too-many-sigops
  bad-txns-vin-empty
  bad-txns-vout-empty
  bad-txns-vout-negative
  ```

Tree-SHA512: 05407f4a953fbd7c44c08bb49bb989cefd39a2b05ea00f5b3c92197a3f05e1b302f789e33832445734220e1c333d133aba385740b77b84139b170c583471ce20
…CVE disclosure

c4b0c08 Update tx-size-small comment with relevant CVE disclosure (Gregory Sanders)

Pull request description:

  Code first introduced under bitcoin#11423 with essentially no description and no discussion.

ACKs for top commit:
  MarcoFalke:
    ACK c4b0c08
  fanquake:
    ACK c4b0c08

Tree-SHA512: 95d5c92998b8b1e944c477dbaee265b62612b6e815099ab31d9ff580b4dff777abaf7f326a284644709f918aa1510412d62310689b1250ef6e64de7b19ca9f71
107e030 build: make protobuf optional in depends (fanquake)
ff6122f doc: clarify protobuf build requirements (fanquake)

Pull request description:

  As mentioned by dongcarl in bitcoin#15584 (comment), make building `protobuf` optional in depends. With this change it will only be built if you pass `PROTOBUF=1`.

ACKs for top commit:
  laanwj:
    code review ACK 107e030
  Sjors:
    tACK 107e030 on macOS 10.14. When I build depends with `PROTOBUF=1` then `./configure` has `bip70` enabled.

Tree-SHA512: 49bc247a6879aaf55b943a3d0b930544ddef1e69a481955a8bebe0b02c9ad0fe168b93025f34168334cef34bb567478eb98eacab62ba909f2f64fb21119c71b8
@UdjinM6 UdjinM6 added this to the 18 milestone Jan 24, 2022
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.

LGTM, utACK

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, up to Udjin whether it's blocking

"""Allows simple construction of a certain kind of invalid tx. Base class to be subclassed."""
__metaclass__ = abc.ABCMeta

# The expected error code given by bitcoind upon submission of the tx.
Copy link
Member

Choose a reason for hiding this comment

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

nit: dashify

@UdjinM6
Copy link

UdjinM6 commented Jan 24, 2022

We have a couple of these in our code already 🙈 should be ok to fix them all in a separate cleanup PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants