Skip to content

Conversation

@Munkybooty
Copy link

No description provided.

@github-actions
Copy link

This pull request has conflicts, please rebase.

Copy link

@UdjinM6 UdjinM6 Nov 29, 2021

Choose a reason for hiding this comment

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

16402: requires 11882 and 15891

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@PastaPastaPasta For 11882 you have a comment on the spreadsheet about us not wanting/needing it.

Copy link
Member

Choose a reason for hiding this comment

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

that was simply a "don't need", and we would probably have to mildly adjust logic (all networks should have fallback fee enabled), but I guess it makes sense to backport it now

Copy link

Choose a reason for hiding this comment

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

16293: missing whitespaces here and in other defs below e.g.

Suggested change
def test_many_inputs_fee(self):
def test_many_inputs_fee(self):

@github-actions
Copy link

This pull request has conflicts, please rebase.

@github-actions
Copy link

This pull request has conflicts, please rebase.

Copy link
Member

Choose a reason for hiding this comment

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

Why can't we do changes in upstream build-aux/m4/bitcoin_qt.m4? imo let's drop this PR if it's partial

Copy link
Author

Choose a reason for hiding this comment

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

The changes in build-aux/m4/bitcoin_qt.m4 are in a part of the file we do not have

@github-actions
Copy link

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

#4643
conflictable files: src/rpc/rawtransaction.cpp

fanquake and others added 3 commits January 24, 2022 11:09
fa56b21 doc: Update bips 35, 37 and 111 status (MarcoFalke)

Pull request description:

  Follow-up to

  * bitcoin#16152: Disable bloom filtering by default

ACKs for top commit:
  laanwj:
    ACK fa56b21, thanks for keeping this file up to date
  fanquake:
    ACK fa56b21

Tree-SHA512: 50c17067abbba096e8604b8abccbd0474ecc2dca973935751720c79a023a2d517bb025bb6c36d4fb0d29b7338322af7ae1c0d5a9aaf2712000d9d336c898c204
248e22b depends: disable unused Qt features (fanquake)

Pull request description:

  Related to bitcoin#16354. Kept separate from bitcoin#16370, because:

  > QT is a monster 😂 - dongcarl in #bitcoin-builds

  I've done some basic testing on `macOS 10.14` and `Debian 9.9` so far. Would be good to have someone test on Windows.

  I was thinking about adding some inline documentation, i.e info about where to find the lists of Qt features & libraries, as well as breaking the flags up so that it's clearer which libraries we are supplying, which we rely on Qt for etc. Could go towards addressing  some of`2` in bitcoin#16354.

ACKs for top commit:
  sipsorcery:
    tACK 248e22b (Windows 10 test only)
  laanwj:
    ACK 248e22b

Tree-SHA512: 2cdcea8d268de21d355a7625c4d352f65728df0b8d8cc0f396aca676f42099a819f95652dfbfc665c991ba12c52735c1e9b693df4b12e3ee178fd39356fba8e0
07e01d6 rpc: sendrawtransaction unconditionality/privacy note (Jon Atack)

Pull request description:

  In sendrawtransaction RPCHelpMan, mention unconditionality and privacy as per http://www.erisian.com.au/bitcoin-core-dev/log-2019-07-25.html#l-522

  before

  ```
  $ bitcoin-cli help sendrawtransaction
  sendrawtransaction "hexstring" ( maxfeerate )

  Submits raw transaction (serialized, hex-encoded) to local node and network.

  Also see createrawtransaction and signrawtransactionwithkey calls.

  (...)
  ```

  after

  ```
  $ bitcoin-cli help sendrawtransaction
  sendrawtransaction "hexstring" ( maxfeerate )

  Submit a raw transaction (serialized, hex-encoded) to local node and network.

  Note that the transaction will be sent unconditionally to all peers, so using this
  for manual rebroadcast may degrade privacy by leaking the transaction's origin, as
  nodes will normally not rebroadcast non-wallet transactions already in their mempool.

  Also see createrawtransaction and signrawtransactionwithkey calls.

  (...)
  ```

ACKs for top commit:
  promag:
    ACK 07e01d6.
  laanwj:
    ACK 07e01d6

Tree-SHA512: 427b3ca29384eef271eb496b7b14e883220863543a536ddeb31940aaffd52ea0b607d929d50f2b7958514105ef7823fa05c1ee381d4a432808753c06bd97af58
@Munkybooty Munkybooty force-pushed the backports-0.19-pr7 branch 6 times, most recently from bcf99fc to 9f70fa8 Compare January 24, 2022 21:48
@Munkybooty Munkybooty force-pushed the backports-0.19-pr7 branch 5 times, most recently from 556aadf to 0574096 Compare January 25, 2022 00:18
@PastaPastaPasta
Copy link
Member

test/lint/lint-python.sh fails and tests fail

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.

some fixes and even more backports are required to make it work it seems, pls see https://github.com/UdjinM6/dash/commits/pr4575

laanwj and others added 16 commits January 30, 2022 18:29
…let option provided

4057b7a wallet: Recognize -disablewallet option early (Hennadii Stepanov)

Pull request description:

  This PR makes early check for the `-disablewallet` option.

  If `-disablewallet=1`, objects `PaymentServer` and `WalletController` are  nor created.

ACKs for top commit:
  jonasschnelli:
    utACK 4057b7a
  laanwj:
    ACK 4057b7a

Tree-SHA512: 74633cd1eacd0914c73712e6dff190255b5378595cfee7eaeb91e17671fc9120928034739f4ae1c53b86f46c4b400390877241384376b2fc534de326d3ab0944
faa88d0 doc: update labels in CONTRIBUTING.md (MarcoFalke)

Pull request description:

  None of the examples in the "trivial" area are acceptable pull requests, unless they are acceptable in a different area (like "doc" or "log").

  Fix that by removing the "trivial" area.

ACKs for top commit:
  jonatack:
    ACK faa88d0
  fanquake:
    ACK faa88d0 - agree that trivial was pretty useless and that the meaning was unclear. Other changes look fine. Surprised the white space linter hasn't been having a field day in this file.

Tree-SHA512: 6208bcc7c84ad0ca6aeaa2de1901c9da8971aac332b5e7a1194ea7b24fb2d887f988aa22fdfa818e89cbcfd8cb8595ce312525f88c81c5ade484fd7c9bd13d1b
29ee4c4 Specify AM_CPPFLAGS for ZMQ. (Daniel Kraft)

Pull request description:

  When building the ZMQ static library, add `AM_CPPFLAGS` to the library `CPPFLAGS`.  Otherwise, we may miss important flags that are specified elsewhere.  For instance, if `--enable-debug` is passed and
  `-DDEBUG_LOCKORDER` set, then that would not apply to the ZMQ library before (causing potential for hard-to-find bugs).

ACKs for top commit:
  laanwj:
    utACK 29ee4c4

Tree-SHA512: 64085d71ed3f435a6e4df6dc42bda8b6159a4d292d0547c5b38c09d6ac95e976ad1728cd65278bffdd57363f60a58eb762b1171dafbe055cf94ffcd4f66da877
05b56d1 [wallet] Remove CMerkleTx serialization logic (John Newbery)
783a76f [wallet] Flatten CWalletTx class hierarchy (John Newbery)
b3a9d17 [wallet] Move CMerkleTx functions into CWalletTx (John Newbery)

Pull request description:

  CMerkleTx is only used as a base class for
  CWalletTx. It was previously also used for vtxPrev which
  was removed in 93a18a3.

  This PR moves all of the CMerkleTx members and logic
  into CWalletTx. The CMerkleTx class is kept for deserialization
  and serialization of old wallet files.

  This makes the refactor in bitcoin#15931 cleaner.

ACKs for top commit:
  laanwj:
    ACK 05b56d1. Looks good to me.

Tree-SHA512: 3d3a0069ebb536b12a328f1261e7dc55158a71088d445ae4b4ace4142c432dc296f58c8183b1922e54a60b8cc77e9d17c3dce7478294cd68693594baacf2bab3
faf8318 test: Split fundrawtx test into subtests (MarcoFalke)
fa6fba3 test: Make local symbols in run_test members (MarcoFalke)

Pull request description:

  This prevents scope-leak of symbols that are supposed to be local to one test case.

Top commit has no ACKs.

Tree-SHA512: 9b2a4ca2cdd631ef915d2f7e6cd62375df9a0919448350aa6e5ae4aa8a8fe3ba53870f7a9a25a57736894b4e3a45e861018253ed2d57d9a64c2bb65fa270fad8
3f592b8 [QA] add wallet-rbf test (Jonas Schnelli)
8222e05 Disable wallet fallbackfee by default on mainnet (Jonas Schnelli)

Pull request description:

  Removes the default fallback fee on mainnet (but keeps it on testnet/regtest).

  Transactions using the fallbackfee in case the fallback fee has not been set are getting rejected.

Tree-SHA512: e54d2594b7f954e640cc513a18b0bfbe189f15e15bdeed4fe02b7677f939bca1731fef781b073127ffd4ce08a595fb118259b8826cdaa077ff7d5ae9495810db
fa89bad test: Require standard txs in regtest (MarcoFalke)
fa9b419 test: Add test that mainnet requires standard txs (MarcoFalke)
fa613ca chainparams: Remove unused fMineBlocksOnDemand (MarcoFalke)

Pull request description:

  I don't see a reason why regtest should allow non-standard txs, as it makes testing mainnet behaviour such as bitcoin#15846 unnecessarily hard and unintuitive.

  Of course, testnet policy remains unchanged to allow propagation of non-standard txs.

ACKs for top commit:
  ajtowns:
    ACK fa89bad

Tree-SHA512: c4c675affb054868850bd2683aa07f4c741a448cbacb2ea8334191e105f426b0790fe6a468be61e9c5880d24154f7bf1c7075051697172dce92180c1bc3a1c90
fa4a605 Remove wallet settings from chainparams (MarcoFalke)

Pull request description:

  Feels a bit odd to have wallet setting in the chainparams, so remove them from there

ACKs for top commit:
  promag:
    ACK fa4a605, missed s/2018/2019?
  practicalswift:
    utACK fa4a605
  darosior:
    ACK fa4a605

Tree-SHA512: 2b3a5ee85d36af290d7db80bed1339e3c684607f1ce61cc65c906726e9174e40325fb1f67a34d8780f2a61fa39a1785e7c3a1cef5b6d6c364f38db5300cdbe3a
…_allow_fallback_fee

7ba2d57 Fix ListCoins test failure due to unset g_wallet_allow_fallback_fee (Russell Yanofsky)

Pull request description:

  New global variable was introduced in bitcoin#11882 and not setting it causes:

  ```
  wallet/test/wallet_tests.cpp(638): error in "ListCoins": check wallet->CreateTransaction({recipient}, wtx, reservekey, fee, changePos, error, dummy) failed
  wallet/test/wallet_tests.cpp(679): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2]
  wallet/test/wallet_tests.cpp(686): error in "ListCoins": check available.size() == 2 failed [1 != 2]
  wallet/test/wallet_tests.cpp(705): error in "ListCoins": check list.begin()->second.size() == 2 failed [1 != 2]
  ```

  It's possible to reproduce the failure reliably by running:

  ```
  src/test/test_bitcoin --log_level=test_suite --run_test=wallet_tests/ListCoins
  ```
  Failures happen nondeterministically because boost test framework doesn't run tests in a specified order, and tests that run previously can set the global variables and mask the bug.

  This is similar to bugs bitcoin#12150 and bitcoin#12424. Example travis failures are:

  https://travis-ci.org/bitcoin/bitcoin/jobs/348296805#L2676
  https://travis-ci.org/bitcoin/bitcoin/jobs/348362560#L2769
  https://travis-ci.org/bitcoin/bitcoin/jobs/348362563#L2824

Tree-SHA512: ca37b554a75c12ac2d534de62bf74eb9e0b29e4399ebf1fa10053a40887e55e9e7135f754a01e5a67499cc8677ae226542146b370b1e83d08bb63d79ff379073
…let::CreateTransaction

0d101a3 test: Add test for maxtxfee option (MarcoFalke)
1775501 wallet: Remove unreachable code in CreateTransaction (MarcoFalke)
5c1b971 wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (João Barbosa)

Pull request description:

  Follow up to bitcoin#16257, this PR makes `bumpfee` aware of `-maxtxfee`.

  It also prevents dangling locked unspents when calling `fundrawtransaction` - because the previous check was after `LockCoin`.

ACKs for top commit:
  MarcoFalke:
    re-ACK 0d101a3, only change is small test fixup

Tree-SHA512: 3464b24ae7cd4e72ed41438c6661828ba1304af020f05da62720b23668ae734e16cf47c6d97e150cc84ef631ee099b16fc786c858f3d089905845437338fd512
fa79a78 test: Add reorg test to wallet_balance (MarcoFalke)
fad03cd test: Check that wallet txs not in the mempool are untrusted (MarcoFalke)
fa19531 test: Add getunconfirmedbalance test with conflicts (MarcoFalke)
fa464e8 test: Add wallet_balance test for watchonly (MarcoFalke)

Pull request description:

  Second commit can be reviewed with `--ignore-all-space`

ACKs for commit fa79a7:
  jnewbery:
    utACK fa79a78

Tree-SHA512: ec4919a3c93b6dcb35d58e7c65bdffe7f4c8cb87b9287f3679631c1823ef5bd72789f233def94e60c1ab332711601751645566f5997ce250af55b328ed60e917
bb41e63 wallet_balance.py: Prevent edge cases (Steven Roose)

Pull request description:

  I ran into this edge case when running the test on Elements. I had a 0-value output as change.

ACKs for commit bb41e6:

Tree-SHA512: ef4c25289cafcdb4437f11ed537664dff5afedcefab75a46f985d3be70551de2d3bc8e9cfcb22c0f3d7d2eb95ff40df78b8d01dbacbf90c36bca00426937b0a2
@UdjinM6 UdjinM6 added this to the 18 milestone Feb 13, 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

@PastaPastaPasta PastaPastaPasta merged commit 9789a42 into dashpay:develop Feb 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants