Skip to content

Conversation

@PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Mar 5, 2022

I've done a self review, stuff passes locally

laanwj and others added 3 commits March 5, 2022 14:36
3d5d7aa windows: remove call to SetProcessDEPPolicy (fanquake)
f2645c2 windows: Enable heap terminate-on-corruption (fanquake)

Pull request description:

  This PR is currently two separate changes:

  #### Enable heap terminate-on-corruption
  This is default behavior from Windows 8 onwards, however we still support Windows 7, so it should make sense to explicitly enable this. This is also done by projects like tor, chromium etc.

  > Enables the terminate-on-corruption feature. If the heap manager detects an error in any heap used by the process, it calls the Windows Error Reporting service and terminates the process.
  After a process enables this feature, it cannot be disabled.

  More info [here](https://docs.microsoft.com/en-us/windows/win32/api/heapapi/nf-heapapi-heapsetinformation).

  #### Remove call to SetProcessDEPPolicy()

  DEP is always enabled on 64-bit Windows processes, and `SetProcessDEPPolicy()` only works when called from a 32-bit process. I've tested that our current usage always fails ([as expected](https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-setprocessdeppolicy#remarks)) with [ERROR_NOT_SUPPORTED](https://github.com/mirror/mingw-w64/blob/16151c441e89081fd398270bb888511ebef6fb35/mingw-w64-headers/include/error.h#L42).

  Please don't add a "Needs gitian build" tag here yet.

ACKs for top commit:
  sipsorcery:
    ACK 3d5d7aa.
  laanwj:
    ACK 3d5d7aa

Tree-SHA512: 0948bcf165685b6b573f2cd950680c34356b856690de655ced2b93d497e02e7b22aa195c99f6ce33202f182622c67302ff31c98ab51b7d050574af3debdee5ce
…sViewDB

bee88b8 tests: have coins simulation test also use CCoinsViewDB (James O'Beirne)

Pull request description:

  Before this change, the coins simulation test uses a base view of type
  CCoinsViewTest, which has no relevance outside of the unittest suite. Might as
  well reuse this testcase with a more realistic configuration that has
  CCoinsViewDB (i.e. in-memory leveldb) at the bottom of the view structure.

  This adds explicit use of CCoinsViewDB in the unittest suite.

  #### Before change
  ```
  ./src/test/test_bitcoin --run_test=coins_tests --catch_system_errors=no  21.99s user 0.04s system 99% cpu 22.057 total
  ```

  #### After change
  ```
  ./src/test/test_bitcoin --run_test=coins_tests --catch_system_errors=no  78.80s user 0.04s system 100% cpu 1:18.82 total
  ```

ACKs for top commit:
  ryanofsky:
    Code review ACK bee88b8

Tree-SHA512: 75296b2bcbae2f46e780489aafb032592544a15c384d569d016005692fe79fe60d7f05857cf25cc7b0f9ab1c53b47886a6c71cca074a03fb9afec30e1f376858
89fea68 build: don't pass -w when building for Windows (fanquake)

Pull request description:

  This has been around since the introduction of autotools. However at
  this point I'm not sure we'd ever want to suppress all warnings when
  performing a build, and given that CXX FLAGS will have been overriden
  when cross-compiling for Windows (using depends), this would rarely,
  if-ever be used anyways.

  From https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html:
  > -w
  >
  >     Inhibit all warning messages.

ACKs for top commit:
  hebasto:
    ACK 89fea68

Tree-SHA512: 2b5bdef7fff5c87b28199f5822cab3cdf600c90c01a40db5cd85053eef5dcb5816e2e97ff61a30ff94b4f0c6cb7be22beaef34d82235bdf05ff9da865d40b381
@PastaPastaPasta PastaPastaPasta force-pushed the develop-trivial-2022-03-05 branch from 8901df0 to d9d06b8 Compare March 5, 2022 19:41
MarcoFalke and others added 5 commits March 5, 2022 13:48
…n walletcreatefundedpsbt RPC method

d0a3fee Change docs for walletcreatefundedpsbt RPC method (Ivan Vershigora)

Pull request description:

  `sequence` field in the list of inputs currently marked as "required". Actually it can be omitted and it's value depends on `locktime` and `options.replaceable` fields. Just the same as in `createpsbt` call.

ACKs for top commit:
  achow101:
    ACK d0a3fee

Tree-SHA512: 3f429a2c2eea283a47fb5002a99f7e2a5ed6f67df9fd895c1ab938256c48a6497ed6ac2673d8fe8968dfb67b939f4a84570899d9faf52f3abd6ec90c0703d1bd
6457361 qt: Fix QFileDialog for static builds (Hennadii Stepanov)

Pull request description:

  This change partially reverts 248e22b (bitcoin#16386) and makes `QFileDialog`s work again for static builds.

  Fixes bitcoin-core/gui#32.

ACKs for top commit:
  fanquake:
    ACK 6457361. Although it would be good to know exactly _why_ this fixes the issue. At this stage I also don't think this should be a blocker for 0.20.1.
  theuni:
    ACK 6457361

Tree-SHA512: 8ad27e0bcae6debd02f73b7c374743e37d4edd806922b103a2fe494cf2d9930fe9ef3107b5a6c61f3c466cf7462de2641171880398954e7f2c4f417f5bb820d7
7087440 depends: native_ds_store 1.3.0 (fanquake)

Pull request description:

  `ds_store` [now takes advantage](dmgbuild/ds_store@36fb607) of Pythons ability to decode binary [plists](https://docs.python.org/3/library/plistlib.html) (since 3.4), so we can drop its biplist dependency.

  The call to `biplist.Data()` in `custom_dsstore.py` doesn't seem to do anything, and from what I can tell can just be removed. i.e:
  ```diff
  diff --git a/contrib/macdeploy/custom_dsstore.py b/contrib/macdeploy/custom_dsstore.py
  index dc1c188..e475bc6c3 100755
  --- a/contrib/macdeploy/custom_dsstore.py
  +++ b/contrib/macdeploy/custom_dsstore.py
  @@ -47,6 +47,7 @@ alias.volume.disk_image_alias.target.filename = package_name_ns + '.temp.dmg'
   alias.volume.disk_image_alias.target.carbon_path = 'Macintosh HD:Users:\x00bitcoinuser:\x00Documents:\x00bitcoin:\x00bitcoin:\x00' + package_name_ns + '.temp.dmg'
   alias.volume.disk_image_alias.target.posix_path = 'Users/bitcoinuser/Documents/bitcoin/bitcoin/' + package_name_ns + '.temp.dmg'
   alias.target.carbon_path = package_name_ns + ':.background:\x00background.tiff'
  +assert(biplist.Data(alias.to_bytes()) == alias.to_bytes())
   icvp['backgroundImageAlias'] = biplist.Data(alias.to_bytes())
   ds['.']['icvp'] = icvp
  ```

ACKs for top commit:
  laanwj:
    ACK 7087440

Tree-SHA512: 8ba3cf561937efe4a3daae8b0cb4de3bf9e425b3a9244161b09d94ee2b1bd4c3e21315fa70e495b19a052aabdc1731b3b6f346b63272d72d2762ced83237d02f
819d03b refactor: took out unused member functions (Zero)
ed69213 build: enable unused member function diagnostic (Zero)

Pull request description:

  This PR enables the `-Wunused-member-function` compiler diagnostic, as discussed in bitcoin#19702.

  > **Notice**: The `unused-member-function` diagnostic is only available on clang. Therefore, clang should be used to test this PR.

  - [x] Include the `-Wunused-member-function`diagnostic in `./configure.ac`. (ed69213)
  - [x] Resolve the reported warnings. (819d03b)

  Currently, enabling this flag no longer reports the following warnings:

  > **Note**: output from `make 2>&1 | grep "warning: unused member function" | sort | uniq -c`

  ```
  1 index/blockfilterindex.cpp:54:5: warning: unused member function 'DBHeightKey' [-Wunused-member-function]
  2 script/bitcoinconsensus.cpp:50:9: warning: unused member function 'GetType' [-Wunused-member-function]
  1 test/util_tests.cpp:1975:14: warning: unused member function 'operator=' [-Wunused-member-function]
  ```

  All tests have passed locally (from `make check` & `src/test/test_bitcoin`).

  This PR closes bitcoin#19702.

ACKs for top commit:
  practicalswift:
    ACK 819d03b - patch still looks correct :)
  MarcoFalke:
    ACK 819d03b
  pox:
    Tested ACK 819d03b with clang after `make clean`. No unused member function warnings.
  theStack:
    tested ACK 819d03b

Tree-SHA512: 5fdfbbb02b3dc618a90a874a5caa5e01e596fc1d14a209e75a6981f01b253f9bca0cfac8fdd758dd7151986609fb76571c3745124a29cfd4f8cbb8d82a07272e
fa7dc7a fuzz: Bump FuzzedDataProvider.h (MarcoFalke)

Pull request description:

  Fixes bitcoin#21309

ACKs for top commit:
  practicalswift:
    cr ACK fa7dc7a

Tree-SHA512: 7805a943ab173d8f3f1e7e55d76a1cc60f63abf9fbf4d537bfeeb0dcf84ecdfb0417d789bd3f3a0c1603fea38884abb643b4b26c27b262e617e6c9a82894f42e
@PastaPastaPasta PastaPastaPasta requested a review from UdjinM6 March 5, 2022 19:48
@PastaPastaPasta PastaPastaPasta force-pushed the develop-trivial-2022-03-05 branch from d9d06b8 to a462ba9 Compare March 5, 2022 19:48
MarcoFalke and others added 3 commits March 7, 2022 09:42
fa84cae doc: added info to bitcoin.conf doc (Brian Liotti)

Pull request description:

  Should probably be explicitly stated to not make modifications to the conf file while daemon is running. ref bitcoin#11586

  For example, if rpc credentials are modified while bitcoind is running, `bitcoin-cli stop` is unable to stop bitcoind until the original credentials are restored in `bitcoin.conf`

ACKs for top commit:
  rajarshimaitra:
    ACK bitcoin@fa84cae
  Zero-1729:
    ACK fa84cae
  theStack:
    ACK fa84cae 🗄️

Tree-SHA512: f6ddffc25563c0b01e661b6abe43a7909938ad8eca38d6d0e2d4a2ce9fb850e51b54d950ef3118b74b6e340c64fe3f37205861720a2de2933db29782234869bb
…d during unserialization

816f29e addrman: detect on-disk corrupted nNew and nTried during unserialization (Vasil Dimov)

Pull request description:

  Negative `nNew` or `nTried` are not possible during normal operation.
  So, if we read such values during unserialize, report addrman
  corruption.

  Fixes bitcoin#22450

ACKs for top commit:
  MarcoFalke:
    cr ACK 816f29e
  jonatack:
    ACK 816f29e
  lsilva01:
    Code Review ACK bitcoin@816f29e.  This change provides a more accurate description of the error.

Tree-SHA512: 01bdd72d2d86a0ef770a319fee995fd1e147b24a8db84ddb8cd121688e7f94fed73fddc0084758e7183c4f8d08e971f0b1b224f5adb10928a5aa4dbbc8709d74
…darwin on aarch64

54c7754 build: use aarch64 Clang if cross-compiling for darwin on aarch64 (fanquake)

Pull request description:

  If we're cross-compiling for darwin on aarch64 hardware, we need to
  use a Clang that will run on that hardware.

  Only tested in a Linux Docker container (aarch64-unknown-linux-gnu),
  running on an Apple M1 mac-mini (aarch64-apple-darwin20.5.0).

ACKs for top commit:
  hebasto:
    ACK 54c7754, I agree it can be merged (fix in bitcoin#22448 is orthogonal to this one).

Tree-SHA512: 66c530097a5dc072a0a00dc22eb3d4a7d923dfa8ab8160f7c3e395cbe58da324f367548d673c0510606f5225d5d37bb5607a76b1703b8b03ac7d2cceeccbd542
@PastaPastaPasta PastaPastaPasta requested a review from UdjinM6 March 7, 2022 15:43
@PastaPastaPasta PastaPastaPasta force-pushed the develop-trivial-2022-03-05 branch from a462ba9 to b932f26 Compare March 7, 2022 15:43
@UdjinM6 UdjinM6 added this to the 18 milestone Mar 7, 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.

utACK

@UdjinM6 UdjinM6 merged commit afd098c into dashpay:develop Mar 7, 2022
@PastaPastaPasta PastaPastaPasta deleted the develop-trivial-2022-03-05 branch March 8, 2022 00:08
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.

4 participants