The code before the fix only checked the length of R value of the last
signature in the loop, and only for equality (but the length can be
less than 32)
The fixed code checks that length of the R value is less than or equal
to 32 on each iteration of the loop
The BOOST_CHECK(sig.size() <= 70) is merged with sig[3] <= 32 check,
and BOOST_CHECKs are moved outside the loop, for efficiency
3eb6f8b2e61c24a22ea9396d86672307845f35eb wallet (not for backport): improve upgradewallet error messages (Jon Atack)
ca8cd893bb56bf5d455154b0498b1f58f77d20ed wallet: fix and improve upgradewallet error responses (Jon Atack)
99d56e357159c7154f69f28cb5587c5ca20d6594 wallet: fix and improve upgradewallet result responses (Jon Atack)
2498b04ce88696a3216fc38b7d393906b733e8b1 Don't upgrade to HD split if it is already supported (Andrew Chow)
c46c18b788cb0862aafbb116fd37936cbed6a431 wallet: refactor GetClosestWalletFeature() (Jon Atack)
Pull request description:
This follows up on #18836 and #20282 to fix and improve the as-yet unreleased `upgradewallet` feature and also implement review follow-up in https://github.com/bitcoin/bitcoin/pull/18836#discussion_r519328607.
This PR fixes 4 upgradewallet issues:
- this bug: https://github.com/bitcoin/bitcoin/pull/20403#discussion_r526063920
- it returns nothing in the absence of an RPC error, which isn't reassuring for users
- it returns the same thing both in the case of a successful upgrade and when no upgrade took place
- the error message object is currently dead code
This PR fixes the above and provides:
...user feedback to not silently return without upgrading
```
{
"wallet_name": "disable private keys",
"previous_version": 169900,
"current_version": 169900,
"result": "Already at latest version. Wallet version unchanged."
}
```
...better feedback after successfully upgrading
```
{
"wallet_name": "watch-only",
"previous_version": 159900,
"current_version": 169900,
"result": "Wallet upgraded successfully from version 159900 to version 169900."
}
```
...helpful error responses
```
{
"wallet_name": "blank",
"previous_version": 169900,
"current_version": 169900,
"error": "Cannot downgrade wallet from version 169900 to version 159900. Wallet version unchanged."
}
{
"wallet_name": "blank",
"previous_version": 130000,
"current_version": 130000,
"error": "Cannot upgrade a non HD split wallet from version 130000 to version 169899 without upgrading to support pre-split keypool. Please use version 169900 or no version specified."
}
```
updated help:
```
upgradewallet ( version )
Upgrade the wallet. Upgrades to the latest version if no version number is specified.
New keys may be generated and a new wallet backup will need to be made.
Arguments:
1. version (numeric, optional, default=169900) The version number to upgrade to. Default is the latest wallet version.
Result:
{ (json object)
"wallet_name" : "str", (string) Name of wallet this operation was performed on
"previous_version" : n, (numeric) Version of wallet before this operation
"current_version" : n, (numeric) Version of wallet after this operation
"result" : "str", (string, optional) Description of result, if no error
"error" : "str" (string, optional) Error message (if there is one)
}
```
ACKs for top commit:
achow101:
ACK 3eb6f8b
MarcoFalke:
review ACK 3eb6f8b2e61c24a22ea9396d86672307845f35eb 🛡
Tree-SHA512: b767314069e26b5933b123acfea6aa40708507f504bdb22884da020a4ca1332af38a7072b061e36281533af9f4e236d94d3c129daf6fe5b55241127537038eed
e95aaefe2540cb76969818fcc2ff77d33448ed5a build: Avoid secp256k1.h include from system (Niklas Gögge)
Pull request description:
While building i ran into an error because i had a version of `secp256k1.h` under `/usr/local/include` that was incompatible with the secp256k1 code in the repository. This caused a problem because `$(BOOST_CPPFLAGS)` contained `-I/usr/local/include` and the include paths are searched by the compiler in order from left to right, so in the end `$(BITCOIN_INCLUDES)` contained `-I/usr/local/include` before `-I$(srcdir)/secp256k1/include` which caused the compiler to find `secp256k1.h` under `/usr/local/include`.
Looking at git blame i am wondering how this has not happened to anyone else in several years: cb89e18845/src/Makefile.am (L25)
I am on macOS 10.15.
ACKs for top commit:
laanwj:
Code review ACK e95aaefe2540cb76969818fcc2ff77d33448ed5a
hebasto:
ACK e95aaefe2540cb76969818fcc2ff77d33448ed5a, tested on macOS 11 Big Sur by adding `#error` into `/usr/local/include/secp256k1.h`.
Tree-SHA512: 1f0b395725936c179ab60dee3582ec7b21e2f9c0f1895e160d84a487cf0db16d0c7aa47d05800e0aded31685b4362056cac9b9ecca1bb8c308a4c5a810e8dc1d
fa05d19bd6ba619bb3f9aabc05c439cd18d34544 test: Fix intermittent issue in mempool_compatibility (MarcoFalke)
Pull request description:
Fixes https://cirrus-ci.com/task/5141306890518528?command=ci#L6076
The version is too old to understand getmempoolinfo()[loaded], so it is never called when starting (See https://cirrus-ci.com/task/5141306890518528?command=ci#L5541)
ACKs for top commit:
achow101:
ACK fa05d19bd6ba619bb3f9aabc05c439cd18d34544
Tree-SHA512: e912d5dff6236d2d4ac608f9f3b3e255cc2b611f36d79bfe4e2a940709833a947c682d7703327887e1753eab30b95eb2615d7e2c21ce4bca4f089e717cbb51c4
fa69c2c78455fd0dc436018fece9ff7fc83a180d wallet: Do not treat default constructed types as None-type (MarcoFalke)
fac4e136fa3d0fab7fde900a6be921313e16e7a6 refactor: Change pointer to reference because it can not be null (MarcoFalke)
Pull request description:
Equating `0==None` and `""==None` is confusing, unneeded and undocumented
ACKs for top commit:
jonatack:
ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d
achow101:
ACK fa69c2c78455fd0dc436018fece9ff7fc83a180d
Sjors:
tACK fa69c2c78455fd0dc436018fece9ff7fc83a180d modulo `unset`
Tree-SHA512: c4c8d0ad80c6697621d356a9545caf28ca2facc82bb2fa8e70eceb52372d25f0685237c73688c4b01da0e75d213c77c0d45011a8bdfe81ea783d85f045786dac
8f7d1b39efbe65ab2747c593cc3560d4a449a333 Fix QPainter non-determinism on macOS (Andrew Chow)
Pull request description:
Aplies a patch to Qt that fixes the non-determinism by modifying Qt. The source of the non-determinism is how LLVM 8 optimizes qt_intersect_spans when compiling. The particular optimization that seems to be causing the problems is that a temp variable is being added for spans->y. For some reason, when it does this, it chooses different instructions to use when making that variable. We bypass this problem by patching qt_intersect_spans to always make and use this local variable.
Potential alternative to #20436 and #20440
ACKs for top commit:
hebasto:
re-ACK 8f7d1b39efbe65ab2747c593cc3560d4a449a333 ~for merging into the 0.21 branch, but [not into the master](https://github.com/bitcoin/bitcoin/pull/20454) branch.~
fanquake:
ACK 8f7d1b39efbe65ab2747c593cc3560d4a449a333
Tree-SHA512: b0d00a77643554021736524fb64611462ef2ec849a220543c12d99edb0f52f2e8128d2cc61fa82176b7e13b294574774a92d6b649badf8b7630c6d6a7e70ce10
05c10953887bd78af2e21ef6d3c07f90dd885572 test: Add testing of ParseInt/ParseUInt edge cases with leading +/-/0:s (practicalswift)
Pull request description:
Add testing of `ParseInt`/`ParseUInt` edge cases with leading `+`/`-`/`0`:s.
Context: While working on #20457 and #20452 I noticed some edge cases which our unit tests are currently not covering.
ACKs for top commit:
MarcoFalke:
review ACK 05c10953887bd78af2e21ef6d3c07f90dd885572
laanwj:
Code review ACK 05c10953887bd78af2e21ef6d3c07f90dd885572
jonatack:
ACK 05c10953887bd78af2e21ef6d3c07f90dd885572
promag:
Code review ACK 05c10953887bd78af2e21ef6d3c07f90dd885572.
Tree-SHA512: bdfb94d8fa0293512dbba89907cb6dd0f8b1418d878267dd6d49c8c397a0e5b9714441345565d41a6a909a1cda052ef7cccece822f355ff604fcf85f2dc8136f
b1f59d55d920d2b35269b474762f94fec87bfb16 RPC/Wallet: unloadwallet: Clarify docs/error when both the RPC endpoint and wallet_name parameter specify a wallet (Luke Dashjr)
Pull request description:
Just documentation clarifications from #20448
ACKs for top commit:
MarcoFalke:
review ACK b1f59d55d920d2b35269b474762f94fec87bfb16
jonatack:
re-ACK b1f59d55d920d2b35269b474762f94fec87bfb16 per `git diff e8303a0 b1f59d5`
Tree-SHA512: ac068b0aa7ceed49496367fdd9425b59dbba18b56e89b26afc22a6c8ece51f0b92a169cacd55740b1cadab2b32f4f8e8700e609066ab7e59d3b53c7891da585e
0918eb49d5afabdf811da5eeb89f4f2c22d12de2 doc: Document current boost dependency as 1.71.0 (Wladimir J. van der Laan)
Pull request description:
This was forgotten in #19764.
ACKs for top commit:
practicalswift:
ACK 0918eb49d5afabdf811da5eeb89f4f2c22d12de2
fanquake:
ACK 0918eb49d5afabdf811da5eeb89f4f2c22d12de2
Tree-SHA512: bd4a39b96b95adeb725767b283f4cf04d9f0d6ac352e7dc67f88cf575b00a24c6d3f4bf51fe362e0c89aeebb6c7e8e9add9f9f17e843121efd30f8edef6128bc
f190343c96520db254d6689f8f24c9eb36bead6b depends: boost: Specify cflags+compileflags (Carl Dong)
b2328b7989997652af52295a4b2e988e68b8428b depends: boost: Remove unnecessary _archiver_ (Carl Dong)
ab9e047cc2226198d22384c286cb30c4c7d51e83 depends: boost: Cleanup toolset selection (Carl Dong)
86002e7e90d7c78d5a76a159bf17db48aec94f94 depends: boost: Cleanup architecture/address-model (Carl Dong)
d7048fa73fafaf0d89d891d98e51a251a64a1495 depends: boost: Disable all compression (Carl Dong)
9cf2ee54d366c3f5fcfce96244988858599e7e95 depends: boost: Split into non-/native packages (Carl Dong)
a57b49856033f3de4a861367b70aba85aec58cf8 depends: boost: Bump to 1.71.0 (Carl Dong)
800655ff3199782b803e671f5baa77ff90a5591d depends: boost: Refer to version in URL (Carl Dong)
Pull request description:
This PR improves the robustness of our boost package in depends, most notably:
1. Bumps boost from `1.70.0` to `1.71.0`, because `1.71.0`:
1. Removes the need to patch out the unused variable.
f8462a6d27/depends/packages/boost.mk (L36)
Upstream boost patched it out in d20b64cf37, which was first included in the `1.71.0` release
2. Comes packaged with a version of `b2` which allows us to override its `CXX` and `CXXFLAGS`. Previously, choosing a toolset while building `b2` such as `clang` or `gcc` would force `b2`'s build system to invoke the compiler as a bare, hardcoded `clang` or `gcc`. However, our `depends` build system often want to customize this behaviour, adding extra flags or invoking the compiler by an alternate name. So this is useful.
1. Commit where `CXX` was introduced: 374f96516a
2. Commit where `CXXFLAGS` was introduced: 5d49abc1f2
2. The boost package is now split into `native_b2` and `boost`, better representing what actually happens.
- In our `depends` build system, we have a distinction between `native` packages and non-`native` packages. The output of `native` packages are meant to be used on the machine that's performing the build, and the output of non-`native` packages are meant to be used on/for the machine that will ultimately be running bitcoin. Previously, `boost` existed in `depends` as a non-`native` package, but that's partly inaccurate because the `./bootstrap.sh` invocation in its `$(package)_config_cmds` stage actually produced a binary called `b2`, which is run on the machine that's performing the build. This means that `b2` is a `native` package which is being built in an environment set up for the non-`native` package `boost`. This reveals a hidden unintended behavior in our `depends` build system: for linux->darwin cross builds, we use `gcc` for `native` packages, and `clang` for non-`native` packages. But `b2` was actually being built using `clang`, since it was being built in an environment set up for non-`native` packages.
theuni you might be interested in taking a look
ACKs for top commit:
laanwj:
Concept and code review ACK f190343c96520db254d6689f8f24c9eb36bead6b
Tree-SHA512: f8b728a34da4f0a9a985a819a5762f2fc2689ea24c7eba1d24d26dfbd4c59f202227c699b0a4069dab10b6329cf9f4c6dd95082685776ee43dd5f7b659acdef1
Fixes the compile error when used inside operator[]:
./chain.h:404:23: error: C++11 only allows consecutive left square brackets when introducing an attribute
return (*this)[Assert(pindex)->nHeight] == pindex;
^
This means we'll get build output like this when building with DEBUG=1:
g++ -c -pipe -ffunction-sections -O2 -fPIC -std=c++11 -fno-exceptions <lots more> ../../corelib/kernel/qcoreapplication.cpp
rather than just:
compiling ../../corelib/kernel/qcoreapplication.cpp
fa7a4385d08797876de9a05ae8a6fde18e36bd67 ci: Fix doc typos in .cirrus.yml (MarcoFalke)
fa73674738b0b5a271ae387e2ebd76d747e02d91 ci: Run i686 centos ci config on cirrus (MarcoFalke)
fa1f949a4dc2568456f3ba30bbd8ecbc78a423ca ci: Run nowallet ci config on cirrus (MarcoFalke)
Pull request description:
Travis CI Org is shutting down, so move the configs to cirrus ci
ACKs for top commit:
practicalswift:
ACK fa7a4385d08797876de9a05ae8a6fde18e36bd67: patch looks correct
Tree-SHA512: 1b7125c7f0d2288931fb8c5e90345891d5f7c1f00c4af136afbeb36bd2836f72920a1877dacc7be787e2c8d84de2a733c1ca71931e5c101b222c1fea588619b3
Aplies a patch to Qt that fixes the non-determinism by modifying Qt. The
source of the non-determinism is how LLVM 8 optimizes qt_intersect_spans
when compiling. The particular optimization that seems to be causing the
problems is that a temp variable is being added for spans->y. For some
reason, when it does this, it chooses different instructions to use when
making that variable. We bypass this problem by patching
qt_intersect_spans to always make and use this local variable.
