c0bf667912 index: add [nodiscard] attribute to functions writing to the db (furszy)
eef595560e index: coinstats reorg, fail when block cannot be reversed (furszy)
Pull request description:
Found it while reviewing https://github.com/bitcoin/bitcoin/pull/24230#discussion_r1310863359.
During a reorg, continuing execution when a block cannot be reversed leaves the
coinstats index in an inconsistent state.
This was surely overlooked when 'CustomRewind' was implemented.
ACKs for top commit:
ryanofsky:
Code review ACK c0bf667912. Only change since last review is new commit adding [[nodiscard]]
Tree-SHA512: f4fc8522508d23e4fff09a29c935971819b1bd3b2a260e08e2e2b72f9340980d74fbec742a58fe216baf61d27de057c7c8300e8fa075f8507cd1227f128af909
Before this commit the V2Transport::m_send_buffer is used to store the
garbage:
* During MAYBE_V1 state, it's there despite not being sent.
* During AWAITING_KEY state, while it is being sent.
* At the end of the AWAITING_KEY state it cannot be wiped as it's still
needed to compute the garbage authentication packet.
Change this by introducing a separate m_send_garbage field, taking over
the first and last role listed above. This means the garbage is only in
the send buffer when it's actually being sent, removing a few special
cases related to this.
This removes the ability for BIP324Cipher to generate its own key, moving that
responsibility to the caller (mostly, V2Transport). This allows us to write
the random-key V2Transport constructor by delegating to the explicit-key one.
8d6228fc1f consensus/validation.h: remove needless GetTransactionOutputWeight helper (Antoine Poinsot)
Pull request description:
Introduced in #26567. My bad. Thanks AJ for noticing.
ACKs for top commit:
ajtowns:
utACK 8d6228fc1f
Tree-SHA512: cf13647b4aac82fb6a54ae0338e3928e9bdf226ed4f5e91d529996328471744132db2bee9676e0b3f40a8bbe0e0ca51a9e5f91560a84e0f33597290551a1ee18
e73d2a8018 refactor: remove clientversion include from dbwrapper.h (Cory Fields)
4240a082b8 refactor: Use DataStream now that version/type are unused (Cory Fields)
f15f790618 Remove version/hashing options from CBlockLocator/CDiskBlockIndex (Cory Fields)
Pull request description:
This is also a much simpler replacement for #28327.
There are version fields in `CBlockLocator` and `CDiskBlockIndex` that have always been written but discarded when read.
I intended to convert them to use SerParams as introduced by #25284, which [ended up looking like this](3e3af45165). However because we don't currently have any definition of what a hash value would mean for either one of those, and we've never assigned the version field any meaning, I think it's better to just not worry about them.
If we ever need to assign meaning in the future, we can introduce `SerParams` as was done for `CAddress`.
As for the dummy values chosen:
`CDiskBlockIndex::DUMMY_VERSION` was easy as the highest ever client version, and I don't expect any objection there.
`CBlockLocator::DUMMY_VERSION` is hard-coded to the higest _PROTOCOL_ version ever used. This is to avoid a sudden bump that would be visible on the network if CLIENT_VERSION were used instead. In the future, if we ever need to use the value, we can discard anything in the CLIENT_VERSION range (for a few years as needed), as it's quite a bit higher.
While reviewing, I suggest looking at the throwaway `SerParams` commit above as it shows where the call-sites are. I believe that should be enough to convince one's self that hashing is never used.
ACKs for top commit:
TheCharlatan:
Re-ACK e73d2a8018
ajtowns:
reACK e73d2a8018
Tree-SHA512: 45b0dd7c2e918493e2ee92a8e35320ad17991cb8908cb811150a96c5fd584ce177c775baeeb8675a602c90b9ba9203b8cefc0a2a0c6a71078b1d9c2b41e1f3ba
db9888feec net: detect wrong-network V1 talking to V2Transport (Pieter Wuille)
91e1ef8684 test: add unit tests for V2Transport (Pieter Wuille)
297c888997 net: make V2Transport preallocate receive buffer space (Pieter Wuille)
3ffa5fb49e net: make V2Transport send uniformly random number garbage bytes (Pieter Wuille)
0be752d9f8 net: add short message encoding/decoding support to V2Transport (Pieter Wuille)
8da8642062 net: make V2Transport auto-detect incoming V1 and fall back to it (Pieter Wuille)
13a7f01557 net: add V2Transport class with subset of BIP324 functionality (Pieter Wuille)
dc2d7eb810 crypto: Spanify EllSwiftPubKey constructor (Pieter Wuille)
5f4b2c6d79 net: remove unused Transport::SetReceiveVersion (Pieter Wuille)
c3fad1f29d net: add have_next_message argument to Transport::GetBytesToSend() (Pieter Wuille)
Pull request description:
This is part of #27634.
This implements the BIP324 v2 transport (which implements all of what the BIP calls transport layer *and* application layer), though in a non-exposed way. It is tested through an extensive fuzz test, which verifies that v2 transports can talk to v2 transports, and v1 transports can talk to v2 transports, and a unit test that exercises a number of unusual scenarios. The transport is functionally complete, including:
* Autodetection of incoming V1 connections.
* Garbage, both sending and receiving.
* Short message type IDs, both sending and receiving.
* Ignore packets (receiving only, but tested in a unit test).
* Session IDs are visible in `getpeerinfo` output (for manual comparison).
Things that are not included, left for future PRs, are:
* Actually using the v2 transport for connections.
* Support for the `NODE_P2P_V2` service flag.
* Retrying downgrade to V1 when attempted outbound V2 connections immediately fail.
* P2P functional and unit tests
ACKs for top commit:
naumenkogs:
ACK db9888feec
theStack:
re-ACK db9888feec
mzumsande:
Code Review ACK db9888feec
Tree-SHA512: 8906ac1e733a99e1f31c9111055611f706d80bbfc2edf6a07fa6e47b21bb65baacd1ff17993cbbf588063b2f5ad30b3af674a50c7bc8e8ebf4671483a21bbfeb
1580e3be83 fuzz: add ConstructPubKeyBytes function (josibake)
Pull request description:
In https://github.com/bitcoin/bitcoin/pull/28246 and https://github.com/bitcoin/bitcoin/pull/28122 , we add a `PubKeyDestination` and a `V0SilentPaymentsDestination`. Both of these PRs update `fuzz/util.cpp` and need a way to create well-formed pubkeys. Currently in `fuzz/util.cpp`, we have some logic for creating pubkeys in the multisig data provider. This logic is duplicated in #28246 and duplicated again in #28122. Seems much better to have a `ConstructPubKeyBytes` function that both PRs (and any future work) can reuse.
This PR introduces a function to do this and has the existing code use it. While the purpose is to introduce a utility function, the previous multisig code used `ConsumeIntegralInRange(4, 7)` which would have created some uncompressed pubkeys with the prefix 0x05, which is incorrect (see https://bitcoin.stackexchange.com/questions/57855/c-secp256k1-what-do-prefixes-0x06-and-0x07-in-an-uncompressed-public-key-signif)
tldr; using `PickValueFromArray` is more correct as it limits to the set of defined prefixes for compressed and uncompressed pubkeys.
ACKs for top commit:
Sjors:
ACK 1580e3be83
Tree-SHA512: c87c8bcd1f6b3a97ef772be93102efb912811c59f32211cfd531a116f1da8a57c8c6ff106b34f2a2b88d8b34fb5bc30d9f9ed6d2720113ffcaaa2f8d5dc9eb27
During a reorg, continuing execution when a block cannot be
reversed leaves the coinstats index in an inconsistent state,
which was surely overlooked when 'CustomRewind' was implemented.
This introduces a V2Transport with a basic subset of BIP324 functionality:
* no ability to send garbage (but receiving is supported)
* no ability to send decoy packets (but receiving them is supported)
* no support for short message id encoding (neither encoding or decoding)
* no waiting until 12 non-V1 bytes have been received
* (and thus) no detection of V1 connections on the responder side
(on the sender side, detecting V1 is not supported either, but that needs
to be dealt with at a higher layer, by reconnecting)
Before this commit, there are only two possibly outcomes for the "more" prediction
in Transport::GetBytesToSend():
* true: the transport itself has more to send, so the answer is certainly yes.
* false: the transport has nothing further to send, but if vSendMsg has more message(s)
left, that still will result in more wire bytes after the next
SetMessageToSend().
For the BIP324 v2 transport, there will arguably be a third state:
* definitely not: the transport has nothing further to send, but even if vSendMsg has
more messages left, they can't be sent (right now). This happens
before the handshake is complete.
To implement this, we move the entire decision logic to the Transport, by adding a
boolean to GetBytesToSend(), called have_next_message, which informs the transport
whether more messages are available. The return values are still true and false, but
they mean "definitely yes" and "definitely no", rather than "yes" and "maybe".
fa626af3ed Remove unused legacy CHashVerifier (MarcoFalke)
fafa3fc5a6 test: add tests that exercise WithParams() (MarcoFalke)
fac81affb5 Use serialization parameters for CAddress serialization (MarcoFalke)
faec591d64 Support for serialization parameters (MarcoFalke)
fac42e9d35 Rename CSerAction* to Action* (MarcoFalke)
aaaa3fa947 Replace READWRITEAS macro with AsBase wrapping function (MarcoFalke)
Pull request description:
It seems confusing that picking a wrong value for `ADDRV2_FORMAT` could have effects on consensus. (See the docstring of `ADDRV2_FORMAT`).
Fix this by implementing https://github.com/bitcoin/bitcoin/issues/19477#issuecomment-1147421608 .
This may also help with libbitcoinkernel, see https://github.com/bitcoin/bitcoin/pull/28327
ACKs for top commit:
TheCharlatan:
ACK fa626af3ed
ajtowns:
ACK fa626af3ed
Tree-SHA512: 229d379da27308890de212b1fd2b85dac13f3f768413cb56a4b0c2da708f28344d04356ffd75bfcbaa4cabf0b6cc363c4f812a8f1648cff9e436811498278318
583af18fd1 fuzz: introduce and use `ConsumePrivateKey` helper (Sebastian Falbesoner)
Pull request description:
In the course of reviewing BIP324 related PRs I noticed a frequent pattern of creating private keys (`CKey` instances) with data consumed from the fuzz data provider:
```
auto key_data = provider.ConsumeBytes<unsigned char>(32);
key_data.resize(32);
CKey key;
key.Set(key_data.begin(), key_data.end(), /*fCompressedIn=*/true);
```
This PR introduces a corresponding helper `ConsumePrivateKey` in order to deduplicate code. The compressed flag can either be set to a fixed value, or, if `std::nullopt` is passed (=default), is also consumed from the fuzz data provider via `.ConsumeBool()`.
Note that this is not a pure refactor, as some of the replaced call-sites previously consumed a random length (`ConsumeRandomLengthByteVector`) instead of a fixed size of 32 bytes for key data. As far as I can see, there is not much value in using a random size, as in all those cases we can only proceed or do something useful with a valid private key, and key data with sizes other than 32 bytes always lead to invalid keys.
ACKs for top commit:
sipa:
utACK 583af18fd1
brunoerg:
crACK 583af18fd1
Tree-SHA512: 58a178432ba1eb0a2f7597b6700e96477e8b10f429ef642445a52db12e74d81aec307888315b772bfda9610f90df3e1d556cf024c2bef1d520303b12584feaaa
10546a569c wallet: accurately account for the size of the witness stack (Antoine Poinsot)
9b7ec393b8 wallet: use descriptor satisfaction size to estimate inputs size (Antoine Poinsot)
8d870a9873 script/signingprovider: introduce a MultiSigningProvider (Antoine Poinsot)
fa7c46b503 descriptor: introduce a method to get the satisfaction size (Antoine Poinsot)
bdba7667d2 miniscript: introduce a helper to get the maximum witness size (Antoine Poinsot)
4ab382c2cd miniscript: make GetStackSize independent of P2WSH context (Antoine Poinsot)
Pull request description:
The wallet currently estimates the size of a signed input by doing a dry run of the signing logic. This is unnecessary since all outputs we can sign for can be represented by a descriptor, and we can derive the size of a satisfaction ("signature") directly from the descriptor itself.
In addition, the current approach does not generalize well: dry runs of the signing logic are only possible for the most basic scripts. See for instance the discussion in #24149 around that.
This introduces a method to get the maximum size of a satisfaction from a descriptor, and makes the wallet use that instead of the dry-run.
ACKs for top commit:
sipa:
utACK 10546a569c
achow101:
re-ACK 10546a569c
Tree-SHA512: 43ed1529fbd30af709d903c8c5063235e8c6a03b500bc8f144273d6184e23a53edf0fea9ef898ed57d8a40d73208b5d935cc73b94a24fad3ad3c63b3b2027174
fae405556d scripted-diff: Rename CBlockTreeDB -> BlockTreeDB (MarcoFalke)
faf63039cc Fixup style of moved code (MarcoFalke)
fa65111b99 move-only: Move CBlockTreeDB to node/blockstorage (MarcoFalke)
fa8685597e index: Drop legacy -txindex check (MarcoFalke)
fa69148a0a scripted-diff: Use blocks_path where possible (MarcoFalke)
Pull request description:
The only reason for the check was to print a warning about an increase in storage use. Now that 22.x is EOL and everyone should have migrated (or decided to not care about storage use), remove the check.
Also, a move-only commit is included. (Rebased from https://github.com/bitcoin/bitcoin/pull/22242)
ACKs for top commit:
TheCharlatan:
ACK fae405556d, though I lack historical context to really judge the second commit fa8685597e.
stickies-v:
ACK fae405556d
Tree-SHA512: 9da8f48767ae52d8e8e21c09a40c949cc0838794f1856cc5f58a91acd3f00a3bca818c8082242b3fdc9ca5badb09059570bb3870850d3807b75a8e23b5222da1
c0da4f60e2 Squashed 'src/secp256k1/' changes from c545fdc374..199d27cea3 (Pieter Wuille)
Pull request description:
We had previously pulled in a non-released commit along with #27479. The necessary changes have now been released in version 0.4.0, so update to that.
ACKs for top commit:
hebasto:
ACK 0e0fc18c3c, having a zero diff with my local branch that updates the `secp256k1` subtree up to v0.4.0.
fanquake:
ACK 0e0fc18c3c
Tree-SHA512: 8b771e7da89b9cdb7a680b9dd4eb99a6f737b32914b0b62c485b3c484e5438f9f60942030d3072243aaa196da22d2b1fdb3b6a668d75a46e6ac78c9d86b4bd8b
This also cleans up the addrman (de)serialization code paths to only
allow `Disk` serialization. Some unit tests previously forced a
`Network` serialization, which does not make sense, because Bitcoin Core
in production will always `Disk` serialize.
This cleanup idea was suggested by Pieter Wuille and implemented by Anthony
Towns.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Co-authored-by: Anthony Towns <aj@erisian.com.au>
199d27cea3 Merge bitcoin-core/secp256k1#1415: release: Prepare for 0.4.0
16339804c9 release: Prepare for 0.4.0
d9a85065a9 changelog: Catch up in preparation of release
0b4640aedd Merge bitcoin-core/secp256k1#1413: ci: Add `release` job
8659a01714 ci: Add `release` job
f9b38894ba ci: Update `actions/checkout` version
727bec5bc2 Merge bitcoin-core/secp256k1#1414: ci/gha: Add ARM64 QEMU jobs for clang and clang-snapshot
2635068abf ci/gha: Let MSan continue checking after errors in all jobs
e78c7b68eb ci/Dockerfile: Reduce size of Docker image further
2f0d3bbffb ci/Dockerfile: Warn if `ulimit -n` is too high when running Docker
4b8a647ad3 ci/gha: Add ARM64 QEMU jobs for clang and clang-snapshot
6ebe7d2bb3 ci/Dockerfile: Always use versioned clang packages
65c79fe2d0 Merge bitcoin-core/secp256k1#1412: ci: Switch macOS from Ventura to Monterey and add Valgrind
c223d7e33d ci: Switch macOS from Ventura to Monterey and add Valgrind
ea26b71c3a Merge bitcoin-core/secp256k1#1411: ci: Make repetitive command the default one
cce0456304 ci: Make repetitive command the default one
317a4c48f0 ci: Move `git config ...` to `run-in-docker-action`
4d7fe60905 Merge bitcoin-core/secp256k1#1409: ci: Move remained task from Cirrus to GitHub Actions
676ed8f9cf ci: Move "C++ (public headers)" from Cirrus to GitHub Actions
61fc3a2dc8 ci: Move "C++ -fpermissive..." from Cirrus to GitHub Actions
d51fb0a533 ci: Move "MSan" from Cirrus to GitHub Actions
c22ac27529 ci: Move sanitizers task from Cirrus to GitHub Actions
26a989924b Merge bitcoin-core/secp256k1#1410: ci: Use concurrency for pull requests only
ee1be62d84 ci: Use concurrency for pull requests only
6ee14550c8 Merge bitcoin-core/secp256k1#1406: ci, gha: Move more non-x86_64 tasks from Cirrus CI to GitHub Actions
fc3dea29ea ci: Move "ppc64le: Linux..." from Cirrus to GitHub Actions
7782dc8276 ci: Move "ARM64: Linux..." from Cirrus to GitHub Actions
0a16de671c ci: Move "ARM32: Linux..." from Cirrus to GitHub Actions
ea33914e00 ci: Move "s390x (big-endian): Linux..." from Cirrus to GitHub Actions
880be8af99 ci: Move "i686: Linux (Debian stable)" from Cirrus to GiHub Actions
2e6cf9bae5 Merge bitcoin-core/secp256k1#1396: ci, gha: Add "x86_64: Linux (Debian stable)" GitHub Actions job
5373693e45 Merge bitcoin-core/secp256k1#1405: ci: Drop no longer needed workaround
ef9fe959de ci: Drop no longer needed workaround
e10878f58e ci, gha: Drop `driver-opts.network` input for `setup-buildx-action`
4ad4914bd1 ci, gha: Add `retry_builder` Docker image builder
6617a620d9 ci: Remove "x86_64: Linux (Debian stable)" task from Cirrus CI
03c9e6508c ci, gha: Add "x86_64: Linux (Debian stable)" GitHub Actions job
ad3e65d9fe ci: Remove GCC build files and sage to reduce size of Docker image
6b9507adf6 Merge bitcoin-core/secp256k1#1398: ci, gha: Add Windows jobs based on Linux image
87d35f30c0 ci: Rename `cirrus.sh` to more general `ci.sh`
d6281dd008 ci: Remove Windows tasks from Cirrus CI
2b6f9cd546 ci, gha: Add Windows jobs based on Linux image
48b1d939b5 Merge bitcoin-core/secp256k1#1403: ci, gha: Ensure only a single workflow processes `github.ref` at a time
0ba2b94551 Merge bitcoin-core/secp256k1#1373: Add invariant checking for scalars
060e32cb60 Merge bitcoin-core/secp256k1#1401: ci, gha: Run all MSVC tests on Windows natively
de657c2044 Merge bitcoin-core/secp256k1#1062: Removes `_fe_equal_var`, and unwanted `_fe_normalize_weak` calls (in tests)
bcffeb14bc Merge bitcoin-core/secp256k1#1404: ci: Remove "arm64: macOS Ventura" task from Cirrus CI
c2f6435802 ci: Add comment about switching macOS to M1 on GHA later
4a24fae0bc ci: Remove "arm64: macOS Ventura" task from Cirrus CI
b0886fd35c ci, gha: Ensure only a single workflow processes `github.ref` at a time
3d05c86d63 Merge bitcoin-core/secp256k1#1394: ci, gha: Run "x86_64: macOS Ventura" job on GitHub Actions
d78bec7001 ci: Remove Windows MSVC tasks from Cirrus CI
3545dc2b9b ci, gha: Run all MSVC tests on Windows natively
5d8fa825e2 Merge bitcoin-core/secp256k1#1274: test: Silent noisy clang warnings about Valgrind code on macOS x86_64
8e54a346d2 ci, gha: Run "x86_64: macOS Ventura" job on GitHub Actions
b327abfcea Merge bitcoin-core/secp256k1#1402: ci: Use Homebrew's gcc in native macOS task
d62db57427 ci: Use Homebrew's gcc in native macOS task
54058d16fe field: remove `secp256k1_fe_equal_var`
bb4efd6404 tests: remove unwanted `secp256k1_fe_normalize_weak` call
eedd781085 Merge bitcoin-core/secp256k1#1348: tighten group magnitude limits, save normalize_weak calls in group add methods (revival of #1032)
b2f6712dd3 Merge bitcoin-core/secp256k1#1400: ctimetests: Use new SECP256K1_CHECKMEM macros also for ellswift
9c91ea41b1 ci: Enable ellswift module where it's missing
db32a24761 ctimetests: Use new SECP256K1_CHECKMEM macros also for ellswift
ce765a5b8e Merge bitcoin-core/secp256k1#1399: ci, gha: Run "SageMath prover" job on GitHub Actions
8408dfdc4c Revert "ci: Run sage prover on CI"
c8d9914fb1 ci, gha: Run "SageMath prover" job on GitHub Actions
8d2960c8e2 Merge bitcoin-core/secp256k1#1397: ci: Remove "Windows (VS 2022)" task from Cirrus CI
f1774e5ec4 ci, gha: Make MSVC job presentation more explicit
5ee039bb58 ci: Remove "Windows (VS 2022)" task from Cirrus CI
96294c00fb Merge bitcoin-core/secp256k1#1389: ci: Run "Windows (VS 2022)" job on GitHub Actions
a2f7ccdecc ci: Run "Windows (VS 2022)" job on GitHub Actions
374e2b54e2 Merge bitcoin-core/secp256k1#1290: cmake: Set `ENVIRONMENT` property for examples on Windows
1b13415df9 Merge bitcoin-core/secp256k1#1391: refactor: take use of `secp256k1_scalar_{zero,one}` constants (part 2)
a1bd4971d6 refactor: take use of `secp256k1_scalar_{zero,one}` constants (part 2)
b7c685e74a Save _normalize_weak calls in group add methods
c83afa66e0 Tighten group magnitude limits
26392da2fb Merge bitcoin-core/secp256k1#1386: ci: print $ELLSWIFT in cirrus.sh
d23da6d557 use secp256k1_scalar_verify checks
4692478853 ci: print $ELLSWIFT in cirrus.sh
c7d0454932 add verification for scalars
c734c64278 Merge bitcoin-core/secp256k1#1384: build: enable ellswift module via SECP_CONFIG_DEFINES
ad152151b0 update max scalar in scalar_cmov_test and fix schnorrsig_verify exhaustive test
78ca880788 build: enable ellswift module via SECP_CONFIG_DEFINES
0e00fc7d10 Merge bitcoin-core/secp256k1#1383: util: remove unused checked_realloc
b097a466c1 util: remove unused checked_realloc
2bd5f3e618 Merge bitcoin-core/secp256k1#1382: refactor: Drop unused cast
4f8c5bd761 refactor: Drop unused cast
173e8d061a Implement current magnitude assumptions
49afd2f5d8 Take use of _fe_verify_magnitude in field_impl.h
4e9661fc42 Add _fe_verify_magnitude (no-op unless VERIFY is enabled)
690b0fc05a add missing group element invariant checks
175db31149 ci: Drop no longer needed `PATH` variable update on Windows
116d2ab3df cmake: Set `ENVIRONMENT` property for examples on Windows
cef373997c cmake, refactor: Use helper function instead of interface library
747ada3587 test: Silent noisy clang warnings about Valgrind code on macOS x86_64
git-subtree-dir: src/secp256k1
git-subtree-split: 199d27cea32203b224b208627533c2e813cd3b21
df60de770d log: Print error message when coindb is in inconsistent state (Fabian Jahr)
Pull request description:
While doing manual testing on assumeutxo this week I managed to put the coindb into an inconsistent state twice. For a normal user, this can also happen if their computer crashes during a flush or if they try to stop their node during a flush and then get tired of waiting and just shut their computer down or kill the process. It's an edge case but I wouldn't be surprised if this does happen more often when assumeutxo gets used more widely because there might be multiple flushes happening during loading of the UTXO set in the beginning and users may think something is going wrong because of the unexpected wait or they forgot some configs and want to start over quickly.
The problem is, when this happens at first the node starts up normally until it's time to flush again and then it hits an assert that the user can not understand.
```
2023-08-25T16:31:09Z [httpworker.0] [snapshot] 52000000 coins loaded (43.30%, 6768 MB)
2023-08-25T16:31:16Z [httpworker.0] Cache size (7272532192) exceeds total space (7256510300)
2023-08-25T16:31:16Z [httpworker.0] FlushSnapshotToDisk: flushing coins cache (7272 MB) started
Assertion failed: (old_heads[0] == hashBlock), function BatchWrite, file txdb.cpp, line 126.
Abort trap: 6
```
We should at least log an error message that gives users a hint of what the problem is and what they can do to resolve it. I am keeping this separate from the assumeutxo project since this issue can also happen during any regular flush.
ACKs for top commit:
jonatack:
ACK df60de770d
achow101:
ACK df60de770d
ryanofsky:
Code review ACK df60de770d
jamesob:
Code review ACK df60de770d
Tree-SHA512: b546aa0b0323ece2962867a29c38e014ac83ae8f1ded090da2894b4ff2450c05229629c7e8892f7b550cf7def4038a0b4119812e548e11b00c60b1dc3d4276d2
a3b55c94b9 [doc] move comment about AlreadyHaveTx DoS score to the right place (glozow)
3b8c17838a [log] add more logs related to orphan handling (glozow)
51b3275cd1 [log] add category TXPACKAGES for orphanage and package relay (glozow)
a33dde1e41 [log] include wtxid in tx {relay,validation,orphanage} logging (glozow)
Pull request description:
This was taken from #28031 (see #27463 for project tracking).
- Log wtxids in addition to txids when possible. This allows us to track the fate of a transaction from inv to mempool accept/reject through logs.
- Additional orphan-related logging to make testing and debugging easier. Suggested in https://github.com/bitcoin/bitcoin/pull/28031#pullrequestreview-1531022386 and https://github.com/bitcoin/bitcoin/pull/28031#discussion_r1269622220
- Add `TXPACKAGES` category for logging.
- Move a nearby comment block that was in the wrong place.
ACKs for top commit:
instagibbs:
reACK a3b55c94b9
achow101:
ACK a3b55c94b9
brunoerg:
crACK a3b55c94b9
mzumsande:
Code review ACK a3b55c94b9
Tree-SHA512: 21884ef7c2ea2fd006e715574a9dd3e6cbbe8f82d62c6187fe1d39aad5a834051203fda5f355a06ca40c3e2b9561aec50d7c922a662b1edc96f7b552c9f4b24d
32db15450a gui: make '-min' minimize wallet loading dialog (furszy)
Pull request description:
Simple fix for #748.
When '-min' is enabled, no loading dialog should
be presented on screen during startup.
ACKs for top commit:
hebasto:
ACK 32db15450a, tested on Debian 11 + XFCE.
Tree-SHA512: d08060b044938c67e8309db77b49ca645850fc21fdd7d78d5368d336fb9f602dcc66ea398a7505b00bf7d43afa07108347c7260480319fad3ec84cb41332f780
Today, this code only has one spot where it needs well-formed pubkeys,
but future PRs will want to reuse this code.
Add a function which creates a well-formed byte array that can be turned
into a pubkey. It is not required that the pubkey is valid, just that it
can be recognized as a compressed or uncompressed pubkey.
Note: while the main intent of this commit is to wrap the existing
logic into a function, it also switches to `PickValueFromArray` so that
we are only choosing one of 0x04, 0x06, or 0x07. The previous code,
`ConsumeIntegralInRange` would have also picked 0x05, which is not
definied in the context of compressed vs uncompressed keys.
See https://bitcoin.stackexchange.com/questions/57855/c-secp256k1-what-do-prefixes-0x06-and-0x07-in-an-uncompressed-public-key-signif
for more details.
This comment isn't in the right place, as detection of a tx in
recent_rejects would cause the function to exit much earlier.
Move the comment to the right place and tweak the first sentence for
accuracy.
- Whenever a tx is erased. Allows somebody to see which transactions
have been erased due to expiry/overflow, not just how many.
- Whenever a tx is added to a peer's workset.
- AcceptToMemoryPool when a tx is accepted, mirroring the one logged for
a tx received from a peer. This allows someone to see all of the
transactions that are accepted to mempool just by looking for ATMP logs.
- MEMPOOLREJ when a tx is rejected, mirroring the one logged for
a tx received from a peer. This allows someone to see all of the
transaction rejections by looking at MEMPOOLREJ logs.
13eb8aa572 doc: Release notes for testnet defaulting to -acceptnonstdtxn=0 (Anthony Towns)
e1dc15d690 config: default acceptnonstdtxn=0 on all chains (Anthony Towns)
Pull request description:
Changes `-acceptnonstxtxn` to default to 0 on testnet, matching the other chains. Allowing non-standard txs on testnet by default contributed to the difficulties RSK described in #26348: "We see that there are two script paths and, to reduce the script size, a single CHECKMULTISIG is used for the two paths, separating the signer count from the CHECKMULTISIG opcode. This script worked on testnet, because it lacks the standard checks performed in Mainnet."
ACKs for top commit:
MarcoFalke:
lgtm ACK 13eb8aa572
sipa:
utACK 13eb8aa572
instagibbs:
utACK 13eb8aa572
theStack:
Code-review ACK 13eb8aa572
Tree-SHA512: eff7a3f9fc9b94003a730beb96e6f3399bc8b8e93fde4b15f20a11eda61d9a3e076f4423989f98b794b32681abecbc3756a54cd0d37b136e2fb2ffbb47ee7774
