[bug] Missing return value and missing va_end()

[chongzhizhao]

Bug Report

Description

Hi, I was testing cppcheck, a static analysis tool, on dogecoin and found these 2 issues. This is my first time attempting to report bugs, so let me know if they are valid.

1. dogecoin/src/dogecoin-fees.cpp:25: Function with non-void return type has missing return statement.
2. dogecoin/src/zmq/zmqpublishnotifier.cpp:33: va_list "args" not closed by va_end().

Dogecoin Commit ID

4c93783

[chromatic]

Thanks for the report! I'll take a look this weekend unless someone does it before I do.

[patricklodder]

Did some looking in advance

1. dogecoin/src/dogecoin-fees.cpp:25

This is now line 69 and has already been fixed by @chromatic in aeccc23 on 1.14.5-dev

2. dogecoin/src/zmq/zmqpublishnotifier.cpp:33

I think this is still open, I'll wait for the weekend ❤️

[Scerini]

Nice

[chromatic]

Good catch. Bitcoin fixed this in 5ba61f, so pulling in that commit fixes it here (PR #2627).

[patricklodder]

This has been solved for 1.14.5-dev, thank you @chromatic!

cppcheck is interesting by the way, it finds a lot more issues than static analysis from codeql (but it doesn't have the same analysis scope so it's not a replacement). Maybe we should add it to the GH actions run? Thoughts?

[chromatic]

cppcheck caught a couple of things we didn't catch otherwise, so there's value in it. My only concerns are minor: adding additional latency to the GH actions run and maintenance costs of keeping the action running. Neither has been a drag on my work.

[patricklodder]

My only concerns are minor: adding additional latency to the GH actions run and maintenance costs of keeping the action running.

I am not worried about the first because CodeQL already checks dependency code, so we can simply do a native ./configure with system libs and we should be good, because this should then be running faster than CodeQL (i.e. end to end it doesn't have negative impact then)

The second, yes this is a challenge. Though it should be obvious when it is broken, our biggest problems in the past have been that distros change and then the entire CI process breaks. If no one cares, it will stay broken and either become a blocker for PRs, or PRs get merged without testing. I have honestly only seen the latter, so that would be the same result as not doing it? 🤔

[chromatic]

I agree. We always have the option to disable permanently or until it's fixed in the cases where it blocks PRs or its value no longer exceeds its costs.

I just don't want to add something without discussing pros and cons like this. I'm satisfied we have so far.

[patricklodder]

Opened #2631 to explore this further for 1.14.6, so this issue can be closed.