Resolve warnings in consensus lib builds. #5811

evoskuil commented at 1:04 AM on February 21, 2015: none

These changes are limited to issues affecting the consensus library build. As a library intended for developer use it should build cleanly on all intended platforms. These changes should not have any effect on execution.

Fix VS2013 build break in libconsensus. bd018f158a

Make narrowing conversions explicit, eliminate forcing to bool. fd603c994a

Add missing include. f61157b942

Fix extraneous comma warnings. b5a93aa901

Fix unused variable warning in NDEBUG builds. d7c0efcb15

Fix unused parameter warnings. be0f218216

Align comments. cc9c47c458

Fix incorrect explicit cast. 0b3887e441

in src/serialize.h:None in cc9c47c458 outdated

 201 |      }
 202 |      else if (nSize <= std::numeric_limits<unsigned int>::max())
 203 |      {
 204 |          unsigned char chSize = 254;
 205 | -        unsigned int xSize = nSize;
 206 | +        unsigned int xSize = static_cast<unsigned short>(nSize);

theuni commented at 1:25 AM on February 21, 2015:

whoops.

evoskuil commented at 1:33 AM on February 21, 2015: none

@theuni That is understandable, but there is only one line affected by this in the consensus build. Maybe that could be treated as an isolated case, since it's quite trivial.

theuni commented at 1:39 AM on February 21, 2015: member

@evoskuil if anything, as long as we're counting on that behavior, the change should be more like:

#ifdef NDEBUG
#error this is unsupported and very unwise.
#endif

in src/pubkey.cpp:None in 0b3887e441 outdated

  67 | @@ -68,8 +68,8 @@ bool CPubKey::Decompress() {
  68 |          return false;
  69 |  #ifdef USE_SECP256K1
  70 |      int clen = size();
  71 | -    int ret = secp256k1_ecdsa_pubkey_decompress((unsigned char*)begin(), &clen);
  72 | -    assert(ret);
  73 | +    if (secp256k1_ec_pubkey_decompress((unsigned char*)begin(), &clen) == 0)
  74 | +        return false;

evoskuil commented at 1:40 AM on February 21, 2015:

I missed this in my review before writing up my comments. This is a behavior change I meant to discuss before pushing. It doesn't factor in to the consensus call, and appears to be in error. I would appreciate it if someone with more background would comment before I pull this out of the pull request.

theuni commented at 1:42 AM on February 21, 2015:

This code (everything in #ifdef USE_SECP256k1) is currently unused, and should just be removed until it's actually supported.

theuni commented at 1:47 AM on February 21, 2015: member

I suspect that the crashes here are due to some wonky var shadowing issues in serialization. Assuming that's the case, those will need to be addressed before cleaning up unused vars.

Correct inverted test against secp256k1_ecdsa_verify. cc8412857d

Correct hash size qualifiers. 116b7e5012

Remove USE_SECP256K1 conditinal code (not yet supported). 61938ac7fd

Remove NDEBUG condition (NDEBUG builds not yet supported). 9907ed2b3a

evoskuil commented at 6:19 AM on February 21, 2015: none

@theuni In the interest of resolving the other issues, I took up your suggestion and removed the sections of unsupported code: NDEBUG and USE_SECP256K1.

luke-jr commented at 6:26 AM on February 21, 2015: member

Not sure the NDEBUG should be removed. At least it compiles (unlike libsecp256k1 validation).

Change MSC_VER to _MSC_VER in consensus header. 26a2bab4f5

evoskuil commented at 9:13 AM on February 21, 2015: none

Are there any objections to @luke-jr's suggestion that we retain the fix to the NDEBUG warning in the one instance within the consensus code?

Revert removal of NDEBUG condition. 2dad3ac69c

Fix C++11 warning on enum init trailing commas (consensus files). 53dddcc437

gmaxwell commented at 5:36 AM on February 24, 2015: contributor

Verification with libsecp256k1 is not supported at this time. I'm very concerned that more or less intended to be cosmetic changes believed to not effect execution have (above) broken consensus several times. Please take a step back here.

evoskuil commented at 6:00 AM on February 24, 2015: none

@gmaxwell The discussion on the issues with secp256k1 above resulted in the conditional incorporation that library being removed in this pull request.

I'm not sure what you mean by asking me to take a step back. Should I withdraw my pull request because it wasn't initially perfect? I'd be happy to do so if someone else would kindly fix the warnings and breaks.

gmaxwell commented at 6:19 AM on February 24, 2015: contributor

I'm not directing a specific action there. My comment there was to point out that your changes were dangerous and you should probably reconsider your approach, and try to understand the root cause of the prior errors. Review cannot catch all errors, and if you continue to iterate against review and CI you will eventually get something that passes them but still may be broken. E.g. You may want to consider breaking up your changes into smaller sets which are provably equivalent software (e.g. produce identical object code).

With respect to NDEBUG. It is not a supported build configuration at this time. There is specific code in the codebase to actively prevent compilation with NDEBUG set: ''' #if defined(NDEBUG) #error "Bitcoin cannot be compiled without assertions." #endif '''

Assertions are not used for general debugging or error handling in the Bitcoin Core codebase. They are used to test critical invariants which are believed to always be true (on a non faulty compile, on not faulty hardware) where their undetected violation would likely result in a security vulnerability. Historically they were sometimes used in ways in which the expression had side effects and would cause actual failure if removed. Although I believe I have removed all the uses side-effects the system has not been recently audited for them, and some could have been reintroduced (and I note, an overly aggressive avoidance of warnings is one of the things that can result in introducing side effects into assertion statements).

The commit that splits up a statement with NDEBUG ifdefs is not an approach we would be likely to accept, particularly not to silence an written but not read variable warning in an actively unsupported build configuration. On top of a general good practice avoidance of conditional compilation except where strictly needed for portability, performing a byte-saving line split is a messy approach which could lead to serious bugs in the future.

evoskuil commented at 6:38 AM on February 24, 2015: none

There was discussion on the NDBUG question above. I understand how it works. I've reverted it twice to try and satisfy maintainers. I realize it's a crappy fix to a crappy problem. We use a different approach in libbitcoin but I wanted to keep it simple/local here. I really don't care how or if it's resolved.

The consensus lib is presumably for developers. As I said at the outset, it should build cleanly (and not fail to build on major platforms). As a maintainer of one of the platforms that might use this build I've made a considerable effort to point out the issues. Given that these trivial changes are dangerous I will just suppress all warnings and move on.

evoskuil closed this on Feb 24, 2015

theuni commented at 9:52 PM on February 27, 2015: member

@evoskuil Some of these are still useful, I think, if you're interested in attacking them in smaller chunks.

The CSHA256 changes look good now that they're fixed.
Fixed missing include is obviously fine if it's necessary
The extra comma/semicolon fixes look good since iirc they're required for strict standard compliance.

Also personally I like the changes to make casts explicit, but I'm not sure if others would agree there.

I'd drop the unused params and NDEBUG changes though.

DrahtBot locked this on Sep 8, 2021