Bitcoin Core

validation: follow-up nits for lock-free `IsInitialBlockDownload()`

Add `AssertLockHeld(cs_main)` to `ChainstateManager::UpdateIBDStatus()` given `EXCLUSIVE_LOCKS_REQUIRED(cs_main)`.
Fix outdated comment about constness of `ChainstateManager::IsInitialBlockDownload()` (build passes without it).
And since we're touching it, we might as well mark `ChainstateManager::IsInitialBlockDownload()` as `noexcept` now.

Follow-up to #34253.

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>

test: Turn ElapseSteady into SteadyClockContext

tests: log node JSON-RPC errors during test setup

Currently, if the node replies to any command with an error during
the test framework's setup(), we log the generic and not really useful
"Unexpected exception" from the BaseException catch, with no further
information.
This isn't helpful for diagnosing the issue. Fix it by explicitly handling
JSONRPCException and logging the response error message and http status
code.

http-server: guard against crashes from unhandled exceptions

Currently, if an exception is thrown at the top-level HTTP request
handler (prior to invoking the command), the program crashes.

Ideally, each handler should catch all exceptions internally and
be responsible for sanitizing them and crafting the client response.
This is because only the handler knows the correct response format,
which differs per server type. However, because this cannot always
be guaranteed, it is safer to also catch exceptions in the top-level
server code, log the unexpected error, and disconnect the socket.

This both guards against crashes caused by uncaught exceptions and
prevents the client from hanging indefinitely while waiting for a
response that will never arrive.

The following diff can be used to trigger the crash in master
(just run single node functional tests like feature_shutdown.py):
```
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -103,6 +103,9 @@

static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
{
+ static int i = 0; // skip initial requests as they are used in the RPC warmup phase.
+ if (i++ > 3) throw std::runtime_error("error from json rpc handler");
+
// JSONRPC handles only POST
if (req->GetRequestMethod() != HTTPRequest::POST) {
req->WriteReply(HTTP_BAD_METHOD, "JSONRPC server handles only POST requests");

```

Note:
This leaves a TODO in the code because error responses should eventually
be specialized per server type. REST clients expect plain text responses,
while JSON-RPC clients expect a JSON error object.
The TODO is there because this is not consistently enforced everywhere
in the current codebase, and we should tackle them all at once.

miniscript: correct and_v() properties

and_v() must never be 'd'. This is not a bug fix since this was
unreachable in valid Miniscripts: the first sub of an and_v() must be of
type V, which conflicts with (i.e. never has) property 'd'.

script: remove unused SCRIPT_ERR_LAST

It was introduced in ab9edbd6b6eb3efbca11f16fa467c3c0ef905708 and never
used since. It seems it might have been intended to be exposed as part
of a public library interface, which has since been superseded.

The only call site uses SCRIPT_ERR_ERROR_COUNT directly.

remove glozow from trusted keys

lint: Flatten lint image entry points

lint: Add missing --platform=linux to docker build command

Otherwise, this may pick the wrong arch like s390x, if a such a podman
container was most recently used.

See also the CI_IMAGE_PLATFORM setting in the "other" CI, which does the
same.

refactor: Remove remaining std::bind, check via clang-tidy

ci: mount .git dir rw

On merges to master we set LINT_CI_SANITY_CHECK_COMMIT_SIG (when
"GITHUB_REPOSITORY == bitcoin/bitcoin") which runs verify-commits.py.

This requires write access to the .git directory.
Make the mounted .git directory writable.

This is currently not run on PR branches or locally which caused a miss
during review.

test: Check that redundant verack message is ignored

doc: add missing param description to SRD

Co-authored-by: Mark "Murch" Erhardt <murch@murch.one>

ci: remove gnu-getopt usage

This is used for argument parsing in the retry script, however we don't
use the script with any arguments. So remove the unused code, and the
dependency on gnu-getopt.

This came up in the context of adding new CI jobs, where gnu-getopt
might not be available, or working properly. It seemed easier to just
remove the unused code, than look for more workarounds.

doc: Fix LLM nits in net_processing.cpp

Fix a typo and use a named arg, where the LLM suggested it.

scripted-diff: Use references when nullptr is not possible

This allows to skip nullptr checks later in the code, both mentally and literally.

This can be reviewed via the git option:
--word-diff-regex=.

-BEGIN VERIFY SCRIPT-

sed --regexp-extended --in-place '
/^bool PeerManagerImpl::SendMessages\(/,/^}$/ {
s/auto& peer\{maybe_peer\}; .. alias cleaned up .*/Peer\& peer{*maybe_peer};/;
s/peer->/peer./g;
s/\*peer\>/peer/g;

/CNode\* pto\{&node\}; .. alias removed .*/d;
s/pto->/node./g;
s/\*pto\>/node/g;
}
' src/net_processing.cpp

sed --regexp-extended --in-place '
/^void PeerManagerImpl::ProcessMessage\(/,/^}$/ {
/Peer\* peer\{&peer_alias_removed_in_later_commit};/d;
s/peer_alias_removed_in_later_commit/peer/;
s/peer->/peer./g;
s/\*peer\>/peer/g;
}
' src/net_processing.cpp

sed --regexp-extended --in-place '
/^bool PeerManagerImpl::ProcessMessages\(/,/^}$/ {
s/auto& peer\{maybe_peer\}; .. alias cleaned up .*/Peer\& peer{*maybe_peer};/;
s/peer->/peer./g;
s/\*peer\>/peer/g;

/CNode\* pfrom\{&node\}; .. alias removed .*/d;
s/pfrom->/node./g;
s/\*pfrom\>/node/g;
}
' src/net_processing.cpp

-END VERIFY SCRIPT-

refactor: Separate peer/maybe_peer in ProcessMessages and SendMessages

Introducing two names to refer to the peer makes it possible to have one
refer to a non-null reference in a later commit.

refactor: Pass Peer& to ProcessMessage

The peer is never nullptr.

refactor: Pass CNode& to ProcessMessages and SendMessages

The node is never nullptr.

This can be reviewed with the git option:
--word-diff-regex=.

refactor: Make ProcessMessage private again

It is not used in tests anymore.

test: [refactor] Avoid calling private ProcessMessage() function

Calling this low-level function from tests is confusing, and also makes
it harder to change the peer manager implementation.

So juse use the pre-existing test helpers to achieve the same.

[miner] omit dummy extraNonce via IPC

Previously the coinbase transaction generated by our miner code was
not used downstream, because the getblocktemplate RPC excludes it.

Since the Mining IPC interface was introduced in #30200 we do expose
this dummy coinbase transaction. In Stratum v2 several parts of it
are communicated downstream, including the scriptSig.

This commit removes the dummy extraNonce from the coinbase scriptSig
in block templates requested via IPC. This limits the scriptSig
to what is essential for consensus (BIP34) and removes the need for
external mining software to remove the dummy, or even ignore
the scriptSig we provide and generate it some other way. This
could cause problems if a future soft fork requires additional
data to be committed here.

A test is added to verify the new IPC behavior.

It achieves this by introducing an include_dummy_extranonce
option which defaults to false with all test code updated to
set it to true. Because this option is not exposed via IPC,
callers will no longer see it.

The caller needs to ensure that for blocks 1 through 16
they pad the scriptSig in order to avoid bad-cb-length.

Co-authored-by: Anthony Towns <aj@erisian.com.au>

test: clarify getCoinbaseRawTx() comparison

The code comment mistakingly referred to "the deprecated getCoinbaseTx()",
instead of getCoinbaseRawTx. This was missed in d59b4cdb5772917ee13e48552d51662160104b62.

Also rename parse_and_deserialize_coinbase_tx to make it more clear
it refers to the deprecated method.

Finally, this commit drops the getCoinbaseRawTx() call when testing
template inspectors. The coinbase input check here is already covered by
build_coinbase_test.

[doc] Update comments on dummy extraNonces in tests

test: addrman: test self-announcement time penalty handling

Verify that addresses announcing themselves (addr == source) are exempt
from time penalties, while addresses announced by others receive the
expected penalty.