Why would they be confusing? Replacing assert with Assert by accident will compile into the same binary. And replacing Assert with assert will either compile into the same binary or it won't compile at all.

Changed to a macro, thx for the suggestion.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• Overhaul transaction request logic #19184 (Overhaul transaction request logic by sipa)
• Use shared pointers only in validation interface #18354 (Use shared pointers only in validation interface by bvbfan)
• test: Set BIP34Height = 2 for regtest #16333 (test: Set BIP34Height = 2 for regtest by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Assert() seems weird to me too; I don't expect it to return a value. ValidPointer() or having it convert a T* to a T& and using it as SafeDeref(ptr).member or similar would make more sense to me.

Either way, might as well replace the remaining two EnsureChainman() calls in init.cpp and rpc/blockchain.cpp and remove the function from node/context.h entirely by the looks.

This used to be called Ensure() with a deref built in, but I changed it to Assert for the following reason: #18923 (comment)

I am happy to pick either version, as long as reviewers are happy.

replace the remaining two ...

Done in the last force push.

For reviewers who think assert is "weird" and "confusing" and wouldn't "expect it to return a value," I don't see what point you are making. If you saw fun(*ASSERT(ptr)) and had to guess what it was doing, would you actually guess anything other than "this is going to assert ptr is true, dereference it and pass it to fun? If not, what would your guess be?

I like the name ASSERT for this macro, because c++ developers will generally know that a false assert will abort the program. If the name ASSERT is misleading or confusing in a specific way, something longer but more explicit like ABORT_IF_NULL would serve the purpose as well.

As long as this is aborting the program on failure, I think calling this SafeDeref or Ensure or hiding the * operation from the reader is exactly what we don't want here. We don't want a reviewer to look at this code and think that dereferencing the pointer is safe. We want a reviewer to look at this code and see that a pointer is being dereferenced, and that something bad will happen if the pointer is null, and look closely to make sure there are no code paths where it could be null. That is why it is better to keep the *, because reviewers will generally already do this when they see a pointer being dereferenced.

I think the name SafeDeref is specifically bad because I'd expect a function with that name to either safely dereference the pointer or return a default constructed value. I think the name Ensure is specifically bad because there are other cases where Ensure is used in the code and it's basically harmless (throws JSON-RPC a REST exception and returns it to the client).

I mean confusing not because of return value but because now it's valid to use ASSERT() instead of assert().

it's valid to use ASSERT() instead of assert()

Yes, it is valid and should compile to the same binary. (Just like it is fine to replace (++i) with (i++) and it compiles to the same binary). Is there some obvious risk or downside I am missing?

:) no downside. So we could just recommend the new macro for new code? Or add a note for when assert() should be used over ASSERT().

I think we can leave it to code authors to pick whatever version they like more. There really shouldn't be any difference as long as the code compiles.

For reviewers who think assert is "weird" and "confusing" and wouldn't "expect it to return a value,"

I think it's important to keep scrutiny on the changes, not on the people. "Tough on the changes, easy on the people." I've deleted my review.

I've deleted my review

I found the review useful and I adjusted the commit based on the review to include motivation for the change (and fix the typo). Sometimes my commit messages are too short (i.e. empty) when they instead could include further background information and motivation for the changes, so at least I found the review helpful.

re: @jonatack #19277 (comment)

For reviewers who think assert is "weird" and "confusing" and wouldn't "expect it to return a value,"

I think it's important to keep scrutiny on the changes, not on the people. "Tough on the changes, easy on the people." I've deleted my review.

I'm confused. What did your review say? In my email it was a concept ACK with some suggestions not related to this.

Did I phrase my comment badly? I don't think I was scrutinizing anything, just asking a literal, non-rhetorical question to reviewers expressing an opinion I didn't understand. It turned one reviewer clarified and didn't actually have that opinion, so I'm glad I asked. But if I should have asked differently, I'd like to know.

Concept ACK: this will help guide static analyzers and thus increase signal-to-noise in reports. And more generally: explicit guarantees are better than implicit guarantees :)

Sorry for deleting. I was having an "I suck" moment.

Tested ACK fab80fe.

Indeed it's better with macro and templating ninjatsus. Applying

--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -44,6 +44,7 @@ static void WaitForShutdown(NodeContext& node)
 static bool AppInit(int argc, char* argv[])
 {
     NodeContext node;
+    Assert(node.chain);
     node.chain = interfaces::MakeChain(node);

     bool fRet = false;

Gives

src/bitcoind -regtest
Assertion failed: ("node.chain" && check), function operator(), file bitcoind.cpp, line 47.
[1]    90659 abort      src/bitcoind -regtest

If you saw fun(*ASSERT(ptr)) and had to guess what it was doing,

Why would you ever have to guess? I'd think "ugh, what's going on, assert doesn't work like that" and look at the source, and conclude "oh, they're just doing some weird local convention where assert in all caps is slightly different than regular assert". That's not a showstopper, it's just confusing.

If I saw fun(Frobniz(ptr)), I'd think "well, Frobniz is new to me, better look at the source to see what it does" followed by "oh, it asserts if it's null, and otherwise passes it through, cool".

+1 on having Ensure* only do JSON RPC errors. Though, looking at it, the RPC system does both that and CHECK_NONFATAL which throws runtime_error instead, and points directly to where in the source the problem is; might be better to make use of that more consistently instead.

Anything along the lines of ABORT_IF_NULL, if_null_assert, BUG_IF_NULL, fatal_if_null, Require, RequireNonNull or similar wouldn't be confusing to me. fatal_if_null might go nicely with CHECK_NONFATAL. Again, not a showstopper, I just think not overloading standard terms is an improvement.

If you saw fun(*ASSERT(ptr)) and had to guess what it was doing,

Why would you ever have to guess?

Thanks, that's kind of my point. You don't have to guess. If you have any question, you can look at the ASSERT macro and see that it wraps standard assert. But when reading code, people make assumptions about how things work, and when the likely assumptions are wrong, it can be a problem for readability and a future source of bugs. I still don't understand why you are saying something is "confusing" when it doesn't appear there is any actual fact you think someone might be confused about, but it's clear you don't like the aesthetics of overloading the word "assert" here. This is bad, but not as bad as if you thought it caused a practical problem.

Re suggestions:

• ABORT_IF_NULL, BUG_IF_NULL, FATAL_IF_NULL all seem good to me
• REQUIRE or REQUIRE_NOT_NULL seem reasonable to me. My first thought was that these seemed like pointless circumlocutions, trying to avoid the word "assert" for no reason. But maybe there is small difference in meaning since "assert" is something you'd say in english about a statement or fact, while "require" is something you could also say about an object.
• IF_NULL_ASSERT doesn't seem to make logical sense, maybe it was meant to say IF_NULL_ABORT, which would be fine.

Any case, I'm glad we seem to agree there's no readability problem here. I think current PR is fine. My favorite name would probably be ASSERT, followed by one of the names that contains ABORT or FATAL, because REQUIRE doesn't sound scary enough, and doesn't make me think that world may end if the pointer is null. But Marco's the guy putting in the work here, and whatever name he likes I am happy with.

Another reason I like the name Assert is that this plays nicely with our long-term goal to have different levels of checks. See #4576 (comment) for context. It says: "I guess we can have two levels, optional and mandatory checks."

The thread also mentions the danger of building with NDEBUG, which is forbidden since 9b59e3b. Given that it is impossible to build with NDEBUG, no one is testing this configuration and it is effectively unsupported. However, the check is only in one place (main.cpp, or current master: validation and net_processing, or now: check.h). So certainly it is impossible to build bitcoind with NDEBUG. Though, I am less sure about other binaries. And certainly it is possible to (accidentally) build single modules with NDEBUG and then accidentally link them. Haven't checked if this is possible, but the potential risk seems already too high. To fix that, I could imagine a scripted-diff that changes assert to Assert and adds includes for check.h, which would abort compilation under NDEBUG.

Yet another reason I like Assert is that it could make it easier to privide a "pimp-my-assert" version of a plain assert. As an idea it could print additional information, for example:

            tfm::format(std::cerr, "%s:%d (%s)\n"                          \
                                   "Internal bug detected: '%s'\n"         \
                                   "Aborting!\n"                           \
                                   "You may report this issue here: %s\n", \
                __FILE__, __LINE__, __func__,                              \
                (#condition),                                              \
                PACKAGE_BUGREPORT);                                        \

Example taken from #17408.

So using the name Assert instead of e.g. Require would make the least amount of changes in the places where previously assert was used.

Unrelated, but the discussion about Assume was in #16136.