-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Backports 0.15 pr24 #3049
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.
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
Backports 0.15 pr24 #3049
Conversation
|
@PastaPastaPasta We could probably postpone missing PRs and just apply small patches to fix the build https://github.com/UdjinM6/dash/commits/pr3049 if backporting them here would cause some troubles for future backports you (presumably) prepared. (8f8399e1b42c0884f850b3fde50b36a9bbf8f023 would have to be reverted later though) |
774484c to
66f9ef5
Compare
|
Well, that broke quite a few things 🙈Had to backport a part of 9672 bitcoin@891c5ee -> UdjinM6@10d3639 and then apply a bunch of fixes on top, pls see https://github.com/UdjinM6/dash/commits/pr3049. Not sure if I found all the missing parts though, will have a look again tomorrow if Travis isn't happy https://travis-ci.org/UdjinM6/dash/builds/569604147 (but at least tests are passing locally now). |
5b06d5e to
8c6a92f
Compare
|
see added, tests were still failing on your branch however. |
codablock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline comments
src/wallet/rpcwallet.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same !isNull() changes should be applied here. I assume there are more places like this, but I currently don't 100% understand when this change is needed and when not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel as though we should be able to do a scripted diff somehow...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!isNull() is needed when you need to access it via named params. Should probably be applied to Dash specific ones but it's probably ok to do this in a separate PR.
4dc1915 check for null values in rpc args and handle appropriately (Gregory Sanders) 999ef20 importmulti options are optional (Gregory Sanders) a70d025 fixup some rpc param counting for rpc help (Gregory Sanders) Pull request description: Audited where named args will fail to use correct default values or may fail when additional optional arguments are added. Previously for these parameters, it was fine to omit them as positional arguments, but it would trigger UniValue runtime errors to set them to null, or to omit them while passing named parameters with greater positions (which would internally set earlier missing arguments to null). Now null values are treated the same as missing values so these errors do not occur. Included a few other small fixes while working on it. I didn't bother fixing account-based rpc calls. Tree-SHA512: 8baf781a35bd48de7878d4726850a580dab80323d3416c1c146b4fa9062f8a233c03f37e8ae3f3159e9d04a8f39c326627ca64c14e1cb7ce72538f934ab2ae1e
…conf. targets 2aef1f1 [Qt] migrate old fee slider value to new dropbown Always round up (conservative) (Jonas Schnelli) bc1be90 [Qt] replace fee slider with a Dropdown, extend conf. targets (Jonas Schnelli) Tree-SHA512: 53796cf0b434dd3db5d4680dbeb6231a7df8f15d88187178fd4db8917cd7fc60091ce2c1589fd93668fc94bb13f989aba5b7ef3792fa95ee1f9f21a15709e2d3
… in CreateTransaction 1bebfc8 Output Fee Estimation Calculations in CreateTransaction (Alex Morcos) Tree-SHA512: e25a27f7acbbc3a666d5d85da2554c5aaec4c923ee2fdbcfc532c29c6fbdec3c9e0d6ae6044543ecc339e7bd81df09c8d228e0b53a2c5c2dae0f1098c9453272
…d in bitcoin#10284 cc0ed26 Supress struct/class mismatch warnings introduced in bitcoin#10284. (Pavel Janík) Tree-SHA512: 16a6870401b5227c276931841f188479ed5960cf38d8e685f222f58550744c9fcf96a2ea3f2be9a0b1a8d0856a802fc4ec38df7bf90cd5de1f3fe20c4ca15b9d
5e3b7b5 Improve error reporting for estimaterawfee (Alex Morcos) 1fafd70 Add function to report highest estimate target tracked per horizon (Alex Morcos) 9c85b91 Change API to estimaterawfee (Alex Morcos) Tree-SHA512: e624c6e7967e9e48abe49f5818bd674e5710e571cc093029d2f90d39fdfba3c1f30e83bf89f6dce97052b59a7d9636a64642ccfb26effd149c417d0afbed0c0b
0f402b9 Fix rare edge case of paying too many fees when transaction has no change. (Alex Morcos) 253cd7e Only reserve key for scriptChange once in CreateTransaction (Alex Morcos) Pull request description: This is an alternative to bitcoin#10333 See commit messages. The first commit is mostly code move, it just moves the change creation code out of the loop. @instagibbs Tree-SHA512: f16287ae0f0c6f09cf8b1f0db5880bb567ffa74a50898e3d1ef549ba592c6309ae1a9b251739f63a8bb622d48f03ce2dff9e7a57a6bac4afb4b95b0a86613ea8 Signed-off-by: Pasta <[email protected]>
d9d1bd3 nCheckDepth chain height fix (romanornr) Pull request description: ```` if (nCheckDepth <= 0) nCheckDepth = 1000000000; // suffices until the year 19000 if (nCheckDepth > chainActive.Height()) nCheckDepth = chainActive.Height(); ```` These lines confuse me. Correct me if I am wrong, but we can't check any more blocks than we have right? If someone requests <= 0 it get set it into some huge number and then immediately limit it to the chain height in the following statement. ```` if (nCheckDepth > chainActive.Height()) nCheckDepth = chainActive.Height(); ```` when using ````--checkblocks=Z```` When Z is ````0```` or any other negative number, it will check all blocks. I think it should be changed to this maybe. ```` if (nCheckDepth <= 0 || nCheckDepth > chainActive.Height()) nCheckDepth = chainActive.Height(); ```` Which gets rid of that huge number which is confusing for any other altcoins that have a different block time. Tree-SHA512: 8ee0ae5f33b399fa74dc16926709694ccfe1fc8a043cba2f5d00884220ac1b9b13f2df4588041f4133be634e5c7b14f4eebe24294028dafe91581a97dbe627f3
…llet name in `getwalletinfo` and add multiwallet test 3707fcd [wallet] [tests] Add listwallets to multiwallet test (John Newbery) 9508761 [wallet] [rpc] Add listwallets RPC (John Newbery) 4a05715 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery) 09eacee [wallet] fix comment for CWallet::Verify() (John Newbery) Pull request description: - fix comment for CWallet::Verify (cleanup after bitcoin#8694) - expose the wallet name in `getwalletinfo` rpc - add `listwallets` rpc - returns array of wallet names - add functional test for multiwallet using new rpc functionality Tree-SHA512: 52f864726bf8a28421d4f3604a6cb95fffb3f4e19edbce18efaef06142c48dd4adb9e7a65a10de2955c80f13c00803ce27c78ccbc8434d92ef12cd36c4ccb4aa
44eb9d4 [QA] Avoid running multiwallet.py twice (Jonas Schnelli) Pull request description: It's already on L92. Second script execution was introduced in bitcoin#10604 3707fcd (probably rebase issue) Reported by @MarcoFalke Tree-SHA512: cd2873df08e31cbf5b7a43b5e6713b643b758496d4357dcc99d1c3ad2da07e55f6d69996654d17d3f5484219cb5fd4e32da3bfd94701d1137bc955241d285e57
…ions when parameter is a reorg'd block 876e92b Testing: listsinceblock should display all transactions that were affected since the given block, including transactions that were removed due to a reorg. (Karl-Johan Alm) f999c46 listsinceblock: optionally find and list any transactions that were undone due to reorg when requesting a non-main chain block in a new 'removed' array. (Karl-Johan Alm) Pull request description: The following scenario will not notify the caller of the fact `tx0` has been dropped: 1. User 1 receives BTC in tx0 from utxo1 in block aa1. 2. User 2 receives BTC in tx1 from utxo1 (same) in block bb1 3. User 1 sees 2 confirmations at block aa3. 4. Reorg into bb chain. 5. User 1 asks `listsinceblock aa3` and does not see that tx0 is now invalidated. See `listsinceblock.py` commit for related test. The proposed fix is to iterate from the given block down to the fork point, and to check each transaction in the blocks against the wallet, in addition to including all transactions from the fork point to the active chain tip (the current behavior). Any transactions that were present will now also be listed in the `listsinceblock` output in a new `replaced` array. This operation may be a bit heavy but the circumstances (and perceived frequency of occurrence) warrant it, I believe. Example output: ```Python { 'transactions': [], 'replaced': [ { 'walletconflicts': [], 'vout': 1, 'account': '', 'timereceived': 1485234857, 'time': 1485234857, 'amount': '1.00000000', 'bip125-replaceable': 'unknown', 'trusted': False, 'category': 'receive', 'txid': 'ce673859a30dee1d2ebdb3c05f2eea7b1da54baf68f93bb8bfe37c5f09ed22ff', 'address': 'miqEt4kWp9zSizwGGuUWLAmxEcTW9bFUnQ', 'label': '', 'confirmations': -7 } ], 'lastblock': '7a388f27d09e3699102a4ebf81597d974fc4c72093eeaa02adffbbf7527f6715' } ``` I believe this addresses the comment by @luke-jr in bitcoin#9516 (comment) but I could be wrong.. Tree-SHA512: 607b5dcaeccb9dc0d963d3de138c40490f3e923050b29821e6bd513d26beb587bddc748fbb194503fe618cfe34a6ed65d95e8d9c5764a882b6c5f976520cff35
… pass socket as const reference 05e023f Move CloseSocket out of SetSocketNonBlocking and pass SOCKET by const reference in SetSocket* functions (Dag Robole) Pull request description: Rationale: Readability, SetSocketNonBlocking does what it says on the tin. Consistency, More consistent with the rest of the API in this unit. Reusability, SetSocketNonBlocking can also be used by clients that may not want to close the socket on failure. This also moves the responsibility of closing the socket back to the caller that opened it, which in general should know better how and when to close it. Tree-SHA512: 85027137f1b626e2b636549ee38cc757a587adcf464c84be6e65ca16e3b75d7ed1a1b21dd70dbe34c7c5d599af39e53b89932dfe3c74f91a22341ff3af5ea80a
…rors df389bc Change wallet method disabled error text (Russell Yanofsky) e526b3d Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) Pull request description: Raise RPC_WALLET_NOT_SPECIFIED instead of RPC_METHOD_NOT_FOUND when a required wallet filename was not specified in an RPC call. Also raise more specific RPC_WALLET_NOT_FOUND error instead of RPC_INVALID_PARAMETER in case an invalid wallet was specified, for consistency. Tree-SHA512: 6a8d885283f69bcfc28f2e08ac03eff02f9f8160a312ce2a90d868aa52533434fc0b4c4ab86547c2f09392338956df915637eaf7136a4fc105e6c8179f2d0ac8
d84e78e [wallet] Specify wallet name in wallet loading errors (John Newbery) a6da027 Reject invalid wallet files (João Barbosa) 3ef77a0 Reject duplicate wallet filenames (João Barbosa) Pull request description: This PR prevents loading the same wallet more than once in a multi wallet scenario. It also prevents loading with invalid files: non regular files or symlinks. Tree-SHA512: 45bf814096bb788db1c76ff334e679a10686cee7d9c8cd48fe5d924031353ace271f6fb0d4af49a34246d336945515c176920a552be7b9fbe07ab8e00e5f6e5e
b13a68e Squashed 'src/leveldb/' changes from 196962ff0..c521b3ac6 (Pieter Wuille) Pull request description: Includes: * bitcoin-core/leveldb-subtree#2: Prefer std::atomic over MemoryBarrier (Pieter Wuille) * bitcoin-core/leveldb-subtree#5: Move helper functions out of sse4.2 object (Cory Fields) * bitcoin-core/leveldb-subtree#6: Fixes typo (Dimitris Tsapakidis) * bitcoin-core/leveldb-subtree#10: Clean up compile-time warnings (gcc 7.1) (Matt Corallo) * bitcoin-core/leveldb-subtree#11: fixup define checks. Cleans up some oopses from #5 (Cory Fields) Tree-SHA512: 2b88a99a86ed8c74c860de13a123ea7f5424d35d314be564820cf83aaae8308383403f7cd56f17c241cfee4885699796141fed666559c21044eaabaeea073315
…ing fee from recipients 49d903e Eliminate fee overpaying edge case when subtracting fee from recipients (Alex Morcos) Pull request description: I'm not sure if this is the cause of the issue in bitcoin#10034 , but this was a known edge case. I just didn't realize how simple the fix is. Could use a couple more eyes to make sure nothing silly can go wrong here, but if we all agree it's this simple, we can add this as another 0.15 bug fix. Tree-SHA512: db1dd1e83363a3c231267b626d3a388893ee70ba1972056fe2c339c5c9e4fbfd30f7fe837c30cc7be884d454797fd4c619b9d631a8d5eeb55cdb07402a83acb3
…B compactions 8842d1a Add undocumented -forcecompactdb to force LevelDB compactions (Pieter Wuille) Pull request description: Tree-SHA512: de91f3f574f75248fa6e5091089c840957fae5a972ebcd2b89493f7d777d4658560a6f5a3b43ab0c9b2c333ad98f9f185ae224c9caffc1a5e8df369cc414f123
9baca41 build: always attempt to enable targeted sse42 cxxflags (Cory Fields) Pull request description: For depends builds without this, configure thinks that the user has overridden CXXFLAGS manually, when really they've just been set by the config.site. The effect is that warnings and extra cxxflags (sse4.2 for example) were not being added. Tree-SHA512: 9fd615ad0e926bd9d6b541ffcf7fc555e2147e8761f57ff3b5fb5d196c9cef0f26aa99681ff72db8c83c0f9a7ed91f4253f46bab09f2c835044b68047358fa47
…tions to control f135923 Add RPC options for RBF, confirmation target, and conservative fee estimation. (Alex Morcos) f0bf33d Change default fee estimation mode. (Alex Morcos) e0738e3 remove default argument from estimateSmartFee (Alex Morcos) d507c30 Introduce a fee estimate mode. (Alex Morcos) cfaef69 remove default argument from GetMinimumFee (Alex Morcos) Tree-SHA512: 49c3a49a6893790a7e8b4e93a48f123dd5307af26c2017800683b76b4df8fc904ba73402917878676242c7440e3e04288d0c1ff3c2c907418724efc03cedab50
8c6a92f to
c4094c8
Compare
|
Pls also see https://github.com/UdjinM6/dash/commits/pr3049 for more fixes, specifically 4ccac04d2bf8d7ecf95e663d138ca10c28fc12a4, dc83c35f029b61c45ec867396efcf77b88662965 and 55fe1c59514f7d3ab61ba64005d6db839af3a6d7. EDIT: Applying these and #3049 (comment) fixed the build it seems https://travis-ci.org/UdjinM6/dash/builds/570946034. (There is also ab31f3f0413d5f65c8b39d810d2520828d32d049 in that branch but it it looks unrelated.) |
|
all applied |
UdjinM6
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good imo.
Slightly tested ACK
codablock
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
No description provided.