f685a13bef0418663015ea6d8f448f075510c0ec doc: GetTransaction()/getrawtransaction follow-ups to #22383 (John Newbery)
abc57e1f0882a1a2bb20474648419979af6e383d refactor: move `GetTransaction(...)` to node/transaction.cpp (Sebastian Falbesoner)
Pull request description:
~This PR is based on #22383, which should be reviewed first~ (merged by now).
In [yesterday's PR review club session to PR 22383](https://bitcoincore.reviews/22383), the idea of moving the function `GetTransaction(...)` from src/validation.cpp to src/node/transaction.cpp came up. With this, the circular dependency "index/txindex -> validation -> index/txindex" is removed (see change in `lint-circular-dependencies.sh`). Thanks to jnewbery for suggesting and to sipa for providing historical background.
Relevant IRC log:
```
17:52 <jnewbery> Was anyone surprised that GetTransaction() is in validation.cpp? It seems to me that node/transaction.cpp would be a more appropriate place for it.
17:53 <raj_> jnewbery, +1
17:53 <stickies-v> agreed!
17:54 <glozow> jnewbery ya
17:54 <jnewbery> seems weird that validation would call into txindex. I wonder if we remove this function, then validation would no longer need to #include txindex
17:54 <sipa> GetTransaction predates node/transaction.cpp, and even the generic index framework itself :)
17:55 <sipa> (before 0.8, validation itself used the txindex)
17:55 <jnewbery> (and GetTransaction() seems like a natural sibling to BroadcastTransaction(), which is already in node/transaction.cpp)
17:55 <jnewbery> sipa: right, this is not meant as a criticism of course. Just wondering if we can organize things a bit more rationally now that we have better separation between things.
17:55 <sipa> jnewbery: sure, just providing background
17:56 <sipa> seems very reasonable to move it elsewhere now
```
The commit should be trivial to review with `--color-moved`.
ACKs for top commit:
jnewbery:
Code review ACK f685a13bef0418663015ea6d8f448f075510c0ec
rajarshimaitra:
tACK f685a13bef
mjdietzx:
crACK f685a13bef0418663015ea6d8f448f075510c0ec
LarryRuane:
Code review, test ACK f685a13bef0418663015ea6d8f448f075510c0ec
Tree-SHA512: 0e844a6ecb1be04c638b55bc4478c2949549a4fcae01c984eee078de74d176fb19d508fc09360a62ad130677bfa7daf703b67870800e55942838d7313246248c
c3e111a7daf5800026dda4455c737de0412528f1 Reduced number of validations in `tx_validationcache_tests` to keep the run time reasonable. (lucash-dev)
Pull request description:
Following a suggestion in the comments, changed `ValidateCheckInputsForAllFlags` from testing all possible flag combinations to testing a random subset. Also created a new enum constant for the highest flag, so that this test doesn’t keep testing an incomplete subset in case a new flag is added.
Timing for `checkinputs_test`:
```
Before: 6.8s
After: 3.7s
----------------
Saved: 3.1s (45%)
```
This PR was split from #13050. Also see #10026.
ACKs for top commit:
leonardojobim:
tACK c3e111a7da.
kallewoof:
ACK c3e111a7daf5800026dda4455c737de0412528f1
theStack:
re-ACK c3e111a7daf5800026dda4455c737de0412528f1
Tree-SHA512: bef49645bdd4f61ec73cc77a9f028b95d9856db9446d2e7fc9a48867a6f0e94c2c9f150cb771a30fe852db0efb0a1bd15d38b00d712651793ccb59ff6157a7b4
78f4c8b98eada337346ffb206339c3ebae4ff43b prefer to use txindex if available for GetTransaction (Jameson Lopp)
Pull request description:
Fixes#22382
Motivation: prevent excessive disk reads if txindex is enabled.
Worth noting that this could be argued to be less of a bug and more of an issue of undefined behavior. If a user calls GetTransaction with the wrong block hash, what should happen?
ACKs for top commit:
jonatack:
ACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
theStack:
Code review ACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
LarryRuane:
tACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
luke-jr:
utACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
jnewbery:
utACK 78f4c8b98eada337346ffb206339c3ebae4ff43b
rajarshimaitra:
Code review ACK 78f4c8b98e
lsilva01:
Code Review ACK and Tested ACK 78f4c8b98e on Ubuntu 20.04
Tree-SHA512: af7db5b98cb2ae4897b28476b2fa243bf7e6f850750d9347062fe8013c5720986d1a3c808f80098e5289bd84b085de03c81a44e584dc28982f721c223651bfe0
2ebf2fe0e4727a5a57a03f4283bdf1e263855803 test: check for RPC error 'Transaction already in block chain' (-27) (Sebastian Falbesoner)
Pull request description:
This PR adds missing test coverage for the RPC error "Transaction already in block chain" (error code `RPC_VERIFY_ALREADY_IN_CHAIN` = `RPC_TRANSACTION_ALREADY_IN_CHAIN` = -27), which is thrown in the function `BroadcastTransaction` (src/node/transaction.cpp).
ACKs for top commit:
kristapsk:
ACK 2ebf2fe0e4727a5a57a03f4283bdf1e263855803 (ran linter, looked at changes and ran modified test and checked code in `src/node/transaction.cpp`)
darosior:
ACK 2ebf2fe0e4727a5a57a03f4283bdf1e263855803
Tree-SHA512: 8bfbd3ff3da0cb3b8745f69b8ca2377f85fa99f0270750840b60e6ae43b5645c5c59b236993e8b2ad0444ec4171484e4f1ee23fa7e81b79d4222bcb623666fa5
20edf4bcf61e9fa310c3d7f3cac0c80a04df5364 rpc: Return block time in getblockchaininfo (João Barbosa)
Pull request description:
Return tip time in `getblockchaininfo`, for some use cases this can save a call to `getblock`.
ACKs for top commit:
naumenkogs:
ACK 20edf4bcf61e9fa310c3d7f3cac0c80a04df5364
theStack:
re-ACK 20edf4bcf61e9fa310c3d7f3cac0c80a04df5364
0xB10C:
ACK 20edf4bcf61e9fa310c3d7f3cac0c80a04df5364
kristapsk:
ACK 20edf4bcf61e9fa310c3d7f3cac0c80a04df5364
Zero-1729:
re-ACK 20edf4bcf61e9fa310c3d7f3cac0c80a04df5364
Tree-SHA512: 29a920cfff1ef53e0af601c3f93f8f9171f3be47fc84b0fa293cb865b824976e8c1510b17b27d17daf0b8e658dd77d9dc388373395f0919fc4a23cd5019642d5
f036dfbb692c4d44d0f59194d089ed0aa1096347 [addrman] Remove unused test_before_evict argument from Good() (John Newbery)
Pull request description:
This has never been used in the public interface method since it was
introduced in #9037.
ACKs for top commit:
lsilva01:
Tested ACK f036dfbb69 on Ubuntu 20.04.
theStack:
Code-review ACK f036dfbb692c4d44d0f59194d089ed0aa1096347
Tree-SHA512: 98145d9596b4ae1f354cfa561be1a54c6b8057c920e0ac3d4c1d42c9326b2dad2d44320f4171bb701d97088b216760cca8017b84c8b5dcd2b1dc8f158f28066d
fa621ededdfe31a200b77a8787de7e3d2e667aec refactor: Pass script verify flags as uint32_t (MarcoFalke)
Pull request description:
The flags are cast to unsigned in the interpreter anyway, so avoid the confusion (and fuzz crashes) by just passing them as unsigned from the beginning.
Also, the flags are often inverted bit-wise with the `~` operator, which also works on signed integers, but might cause confusion as the sign bit is flipped.
Fixes#22233
ACKs for top commit:
theStack:
Concept and code review ACK fa621ededdfe31a200b77a8787de7e3d2e667aec
kristapsk:
ACK fa621ededdfe31a200b77a8787de7e3d2e667aec
jonatack:
ACK fa621ededdfe31a200b77a8787de7e3d2e667aec
Tree-SHA512: ea0720f32f823fa7f075309978672aa39773c6019d12b6c1c9d611fc1983a76115b7fe2a28d50814673bb6415c311ccc05b99d6e871575fb6900faf75ed17769
fae108ceb53f61d7338ba205873623ede3c1d3be Fix incorrect whitespace in addrman (MarcoFalke)
fa32024d51c098441623e246f304a80f011e29d1 Add missing GUARDED_BY to CAddrMan::insecure_rand (MarcoFalke)
fab755b77f88873f01cbd988051de7ad3f0150de fuzz: Actually use const addrman (MarcoFalke)
fae0c79351ce34186249d44af0c5c9c7521f4b6c refactor: Mark CAddrMan::GetAddr const (MarcoFalke)
fa02934c8c9d290ea4d12683e8680c70967a4d3a refactor: Mark CAddrMan::Select const (MarcoFalke)
Pull request description:
To clarify that a call to this only changes the random state and nothing else.
ACKs for top commit:
jnewbery:
Code review ACK fae108ceb53f61d7338ba205873623ede3c1d3be
theStack:
re-ACK fae108ceb53f61d7338ba205873623ede3c1d3be 🍦
Tree-SHA512: 3ffb211d4715cc3daeb3bfcdb3fcc6b108ca96df5fa565510436fac0e8da86c93b30c9c4aad0563e27d84f615fcd729481072009a4e2360c8b3d40787ab6506a
489ebfd7a16443d8263c048d55622da297df7c39 tests: feature_backwards_compatibility.py test downgrade after upgrade (Andrew Chow)
Pull request description:
After upgrading the node, try to go back to the original version to make sure that using a newer node version does not prevent the wallet file from being downgraded again.
ACKs for top commit:
MarcoFalke:
ACK 489ebfd7a16443d8263c048d55622da297df7c39
Tree-SHA512: 86231de6514b3657912fd9d6621212166fd2b29b591fc97120092c548babcf1d6f50b5bd103b28cecde395a26809134f01c1a198725596c3626420de3fd1f017
2748e8793267126c5b40621d75d1930e358f057e script: prevent UB when computing abs value for num opcode serialize (pierrenn)
Pull request description:
This was reported by practicalswift here #18046
It seems that the original author of the line used a reference to glibc `abs`: https://github.com/lattera/glibc/blob/master/stdlib/abs.c
However depending on some implementation details this can be undefined behavior for unusual values.
A detailed explanation of the UB is provided here : https://stackoverflow.com/questions/17313579/is-there-a-safe-way-to-get-the-unsigned-absolute-value-of-a-signed-integer-with (by [Billy O'Neal](https://twitter.com/malwareminigun))
Simple relevant godbolt example : https://godbolt.org/z/yRwtCG
Thanks!
ACKs for top commit:
sipa:
ACK 2748e8793267126c5b40621d75d1930e358f057e
MarcoFalke:
ACK 2748e8793267126c5b40621d75d1930e358f057e, only checked that the bitcoind binary does not change with clang -O2 🎓
practicalswift:
ACK 2748e8793267126c5b40621d75d1930e358f057e
Tree-SHA512: 539a34c636c2674c66cb6e707d9d0dfdce63f59b5525610ed88da10c9a8d59d81466b111ad63b850660cef3750d732fc7755530c81a2d61f396be0707cd86dec
fabe44e8154a6068d6cba91ec30f00345ed7b275 bench: Start nodes with -nodebuglogfile (MarcoFalke)
Pull request description:
For benchmarking we don't want to depend on the speed of the disk or the amount of debug logging
ACKs for top commit:
fanquake:
ACK fabe44e8154a6068d6cba91ec30f00345ed7b275 - This makes some of these benchmarks significantly faster to run. MempoolEviction total runtime is down from ~46s to 11s on my machine:
Tree-SHA512: d99700901650325896b9115d20b84a27042152f46266f595bf7ea1414528c0b346f4e707a12ee8b8ba99c35cf155e645e67971c1b2a679c4e609c400ff8b08ae
d51f0fa4b7b19281efe65aacf414845c661d0a13 doc: add release notes for 26896 (fanquake)
2b248798d96f794db08b7725730b5fb4e00b9b10 build: remove --enable-upnp-default from configure (fanquake)
02f5a5e7b5fd7ba35e407d4409202a0e0fed003c build: remove --enable-natpmp-default from configure (fanquake)
25a0e8ba0b31d8bd265df0589fe49241a60d0fc2 Remove configure-time setting of DEFAULT_UPNP (fanquake)
06562e5fa771dab275a9cab4914cd64d961a52bc Remove configure-time setting of DEFAULT_NATPMP (fanquake)
Pull request description:
This PR removes the `--enable-upnp-default` and `--enable-natpmp-default` options from configure.
It's odd to me that we maintain configure-time options for setting the default port-forwarding runtime state (but no other similar options), and I'm not sure what use-case it satisfies, that can't be achieved by multiple other means. I also doubt that we'll ever restart using these in release builds, or turning on any of this by default.
I think the only scenario these options would be used is when you want to compile your own binaries (we don't use them in Guix), with port-forwarding on by default, but otherwise can't or don't want to use a `.conf` file, can't or don't want to pass command line options at runtime, and also don't want to modify the source code?
ACKs for top commit:
hebasto:
ACK d51f0fa4b7b19281efe65aacf414845c661d0a13, rebased and comments have been addressed since my recent [review](https://github.com/bitcoin/bitcoin/pull/26896#pullrequestreview-1273910740).
TheCharlatan:
ACK d51f0fa4b7b19281efe65aacf414845c661d0a13
Tree-SHA512: 481decd8bddd8b03b7319591e3acf189f7b6b96c9a9a8c5bc1a3f8ec00d0b8f9b52d2f5c28a298a2ec947cfe9611cfd184e393ccb2e4e21bfce86ca7d4de60d3
2896c412fadbc03916a33028f4f50fd87ac48edb Do not answer GETDATA for to-be-announced tx (Pieter Wuille)
f2f32a3dee9a965c8198f9ddd3aaebc627c273e4 Push down use of cs_main into FindTxForGetData (Pieter Wuille)
c6131bf407c1ada78a0e5509a702bc7da0bfd57d Abstract logic to determine whether to answer tx GETDATA (Pieter Wuille)
Pull request description:
This PR intends to improve transaction-origin privacy.
In general, we should try to not leak information about what transactions we have (recently) learned about before deciding to announce them to our peers. There is a controlled transaction dissemination process that reveals our transactions to peers that has various safeguards for privacy (it's rate-limited, delayed & batched, deterministically sorted, ...), and ideally there is no way to test which transactions we have before that controlled process reveals them. The handling of the `mempool` BIP35 message has protections in this regard as well, as it would be an obvious way to bypass these protections (handled asynchronously after a delay, also deterministically sorted).
However, currently, if we receive a GETDATA for a transaction that we have not yet announced to the requester, we will still respond to it if it was announced to *some* other peer already (because it needs to be in `mapRelay`, which only happens on the first announcement). This is a slight privacy leak.
Thankfully, this seems easy to solve: `setInventontoryTxToSend` keeps track of the txids we have yet to announce to a peer - which almost(*) exactly corresponds to the transactions we know of that we haven't revealed to that peer. By checking whether a txid is in that set before responding to a GETDATA, we can filter these out.
(*) Locally resubmitted or rebroadcasted transactions may end up in setInventoryTxToSend while the peer already knows we have them, which could result in us incorrectly claiming we don't have such transactions if coincidentally requested right after we schedule reannouncing them, but before they're actually INVed. This is made even harder by the fact that filterInventoryKnown will generally keep known reannouncements out of setInventoryTxToSend unless it overflows (which needs 50000 INVs in either direction before it happens).
The condition for responding now becomes:
```
(not in setInventoryTxToSend) AND
(
(in relay map) OR
(
(in mempool) AND
(old enough that it could have expired from relay map) AND
(older than our last getmempool response)
)
)
```
ACKs for top commit:
naumenkogs:
utACK 2896c41
ajtowns:
ACK 2896c412fadbc03916a33028f4f50fd87ac48edb
amitiuttarwar:
code review ACK 2896c412fa
jonatack:
ACK 2896c412fadbc03916 per `git diff 2b3f101 2896c41` only change since previous review is moving the recency check up to be verified first in `FindTxForGetData`, as it was originally in 353a391 (good catch), before looking up the transaction in the relay pool.
jnewbery:
code review ACK 2896c412fadbc03916a33028f4f50fd87ac48edb
Tree-SHA512: e7d5bc006e626f60a2c108a9334f3bbb67205ace04a7450a1e4d4db1d85922a7589e0524500b7b4953762cf70554c4a08eec62c7b38b486cbca3d86321600868
83da576f4416c64b5d520819208a722b2273739a net: use CMessageHeader::HEADER_SIZE, add missing include (Jon Atack)
Pull request description:
as suggested 16 months ago by Gleb Naumenko in https://github.com/bitcoin/bitcoin/pull/15197#issuecomment-456181865.
`static constexpr CMessageHeader::HEADER_SIZE` is already used in this file, `src/net.cpp`, in 2 instances. This commit replaces the remaining 2 integer values in the file with it and adds the explicit include header.
Co-authored by: Gleb Naumenko <naumenko.gs@gmail.com>
ACKs for top commit:
naumenkogs:
utACK 83da576
practicalswift:
ACK 83da576f4416c64b5d520819208a722b2273739a -- patch looks correct
theStack:
ACK 83da576f4416c64b5d520819208a722b2273739a -- verified that its just magic number elimination refactoring and additionally checked that all tests pass 👍
Tree-SHA512: 5b915483bca4ea162c259865a1b615d73b88a1b1db3f82db05f770d10b8a42494d948f5b21badbcce2d9efa5915b8cbb6af83073867c23d2f152c0d35ac37b96
746736639e6d05acdb85c866d4c605c947d4c500 [net processing] Only send a getheaders for one block in an INV (John Newbery)
Pull request description:
Headers-first is the primary method of announcement on the network. If a node fell back sending blocks by inv, it's probably for a re-org. The final block hash provided should be the highest, so send a getheaders and then fetch the blocks we need to catch up.
Sending many GETHEADERS messages to the peer would cause them to send a large number of potentially large HEADERS messages with redundant data, which is a waste of bandwidth.
ACKs for top commit:
sipa:
utACK 746736639e6d05acdb85c866d4c605c947d4c500
mzumsande:
utACK 746736639e6d05acdb85c866d4c605c947d4c500 as per ajtowns' reasoning.
naumenkogs:
utACK 7467366
ajtowns:
ACK 746736639e6d05acdb85c866d4c605c947d4c500
jonatack:
ACK 746736639e6d05acdb85c866d4c605c947d4c500
Tree-SHA512: 59e243b80d3f0873709dfacb2e4ffba34689aad7de31ec7f69a64e0e3a0756235a0150e4082ff5de823949ba4411ee1aed2344b4749b62e0eb1ea906e41f5ea9
80d4423f997e15780bfa3f91bf4b4bf656b8ea45 Test buffered valid message (Troy Giorshev)
Pull request description:
This PR is a tweak of #19302. This sends a valid message.
Additionally, this test includes logging in the same vein as #19272.
ACKs for top commit:
MarcoFalke:
tested ACK 80d4423f997e15780bfa3f91bf4b4bf656b8ea45 (added an assert(false) to observe deterministic coverage) 🌦
gzhao408:
ACK 80d4423f99👊
Tree-SHA512: 3b1aa5ec480a1661917354788923d64595e2886448c9697ec0606a81293e8b4a4642b2b3cc9afb2206ce6f74e5c6d687308c5ad19cb73c5b354d3071ad8496f8
Adds missing changes from p2p_invalid_tx.py but one assert is still disabled
fa4c29bc1d2425f861845bae4f3816d9817e622a test: Add various low-level p2p tests (MarcoFalke)
Pull request description:
ACKs for top commit:
jonatack:
ACK fa4c29bc1d242
Tree-SHA512: 842821b97359d4747c763398f7013415858c18a300cd882887bc812d039b5cbb67b9aa6f68434575dbc3c52f7eb8c43d1b293a59555a7242c0ca615cf44dc0aa
Prior required changes: bitcoin/bitcoin#19438 from
https://github.com/dashpay/dash/pull/5740
## Issue being fixed or feature implemented
Assertion failure:
```
assertion: ok
file: evo/mnhftx.cpp, line: 287
function: AbstractEHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex*)
No debug information available for stacktrace. You should add debug information and then run: dash-qt -printcrashinfo=bvcgc43iinzgc43ijfxgm3ybaadwiyltnawxc5e3ifzxgzlsoruw63ramzqws3dvojstucraebqxg43foj2gs33ohiqg62ykeaqgm2lmmu5cazlwn4xw23timz2hqltdobycyidmnfxgkoragi4docraebthk3tdoruw63r2ebawe43uojqwg5cfjbde2ylomftwk4r2hjjwsz3omfwhgicdjvheqrsnmfxgcz3foi5dur3fordhe33ninqwg2dffbrw63ttoqqegqtmn5rwwslomrsxqkrjbyrelhyaaaaaaaedgkiaaaaaaaadsm4qaaaaaaaa7gpyqaaaaaaaa2njraaaaaaaadkl3caaaaaaaabhxznqaaaaaaadqa2eaaaaaaaax33twaaaaaaabwaihqaaaaaaac7yooqbaaaaaahba45qcaaaaaacwkz2aeaaaaaaeitgeaiaaaaaaaa=
dash-qt: evo/mnhftx.cpp:287: AbstractEHFManager::Signals CMNHFManager::GetFromCache(const CBlockIndex*): Assertion `ok' failed.
```
This can happen in case if Dash Core has been update from v19 (or
earlier v20.alphaX) to v20.0.0 after v20 activation without re-indexing
## What was done?
`CMNHFManager` is visiting missing blocks recursively until reach first
v20 block or first block actually saved in evoDb.
Without changes from bitcoin/bitcoin#19438 there's an other issue:
```
2023-11-27T11:12:10Z POTENTIAL DEADLOCK DETECTED
Previous lock order was:
(2) 'cs_main' in llmq/instantsend.cpp:459 (in thread 'isman')
(1) 'cs_llmq_vbc' in llmq/utils.cpp:711 (in thread 'isman')
Current lock order is:
'cs_dip3list' in qt/masternodelist.cpp:135 (TRY) (in thread 'main')
(1) 'cs_llmq_vbc' in llmq/utils.cpp:719 (in thread 'main')
(2) 'cs_main' in node/blockstorage.cpp:77 (in thread 'main')
Assertion failed: detected inconsistent lock order for 'cs_main' in node/blockstorage.cpp:77 (in thread 'main'), details in debug log.
2023-11-27T11:12:10Z Posix Signal: Aborted
```
## How Has This Been Tested?
Run unit/functional test; run dash-qt on my local backup of problematic
storage (succeed without error); reindex testnet.
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
e48826ad87b4f92261f7433e84f48dac9bd9e5c3 tests: remove ComputeBlockVersion shortcut from versionbits tests (Anthony Towns)
c5f36725e877d8eb492383844f8ef7535466b366 [refactor] Move ComputeBlockVersion into VersionBitsCache (Anthony Towns)
4a69b4dbe0d7f504811b67c399da7e6d11e4f805 [move-only] Move ComputeBlockVersion from validation to versionbits (Anthony Towns)
0cfd6c6a8f929d5567ac41f95c21548f115efee5 [refactor] versionbits: make VersionBitsCache a full class (Anthony Towns)
8ee3e0bed5bf2cd3c7a68ca6ba6c65f7b9a72cca [refactor] rpc/blockchain.cpp: SoftForkPushBack (Anthony Towns)
92f48f360da5f425428b761219301f509826bec4 deploymentinfo: Add DeploymentName() (Anthony Towns)
ea68b3a5729f5d240e968388c4f88acffeb27228 [move-only] Rename versionbitsinfo to deploymentinfo (Anthony Towns)
c64b2c6a0f79369624ae96b2e3d579d50aae4de6 scripted-diff: rename versionbitscache (Anthony Towns)
de55304f6e7a8b607e6b3fc7436de50910747b0c [refactor] Add versionbits deployments to deploymentstatus.h (Anthony Towns)
2b0d291da8f479739ff394dd92801da8c40b9f8e [refactor] Add deploymentstatus.h (Anthony Towns)
eccd736f3dc231ac0306ca763c3b72cf8247230a versionbits: Use dedicated lock instead of cs_main (Anthony Towns)
36a4ba0aaaa9b35185d7178994e36bc02cca9887 versionbits: correct doxygen comments (Anthony Towns)
Pull request description:
Introduces helper functions to make it easy to bury future deployments, along the lines of the suggestion from [11398](https://github.com/bitcoin/bitcoin/pull/11398#issuecomment-335599326) "I would prefer it if a buried deployment wouldn't require all code paths that check the BIP9 status to require changing".
This provides three functions: `DeploymentEnabled()` which tests if a deployment can ever be active, `DeploymentActiveAt()` which checks if a deployment should be enforced in the given block, and `DeploymentActiveAfter()` which checks if a deployment should be enforced in the block following the given block, and overloads all three to work both with buried deployments and versionbits deployments.
This adds a dedicated lock for the versionbits cache, which is acquired internally by the versionbits functions, rather than relying on `cs_main`. It also moves moves versionbitscache into deploymentstatus to avoid a circular dependency with validation.
ACKs for top commit:
jnewbery:
ACK e48826ad87b4f92261f7433e84f48dac9bd9e5c3
gruve-p:
ACK e48826ad87
MarcoFalke:
re-ACK e48826ad87b4f92261f7433e84f48dac9bd9e5c3 🥈
Tree-SHA512: c846ba64436d36f8180046ad551d8b0d9e20509b9bc185aa2639055fc28803dd8ec2d6771ab337e80da0b40009ad959590d5772f84a0bf6199b65190d4155bed
## Issue being fixed or feature implemented
Avoid a crash on -reindex-chainstate.
## What was done?
`ResetBlockFailureFlags` is crashing when `m_chain.Tip()` is null. Call
`ResetBlockFailureFlags` inside `if (!is_coinsview_empty(chainstate))
{...}` block - we know `m_chain.Tip()` is not null there.
## How Has This Been Tested?
Try running a node with `-reindex-chainstate` cmd-line param w/ and
w/out this patch.
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
## Issue being fixed or feature implemented
Our [Release Process document](doc/release-process.md) is not exactly
matched with our [template
issue](https://github.com/dashpay/dash/issues/5694) that created by
copy-paste from previous release.
For the next release just copy test from this document to new issue
## What was done?
[Release Process document](doc/release-process.md) is updated to match
with our real release process.
This document has also detailed instructions for many steps (in compare
to the issue that we use now which is more checklist) to make releasing
process easier for all participant.
## How Has This Been Tested?
Tested on air by 2 last released: v19, v20:
https://github.com/dashpay/dash/issues/5694
## Breaking Changes
N/A
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [x] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone
---------
Co-authored-by: thephez <thephez@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
## Issue being fixed or feature implemented
## What was done?
## How Has This Been Tested?
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
---------
Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
## Issue being fixed or feature implemented
https://github.com/dashpay/dash/pull/4788#discussion_r854468664
noticed while working on #5731
## What was done?
## How Has This Been Tested?
run a node, check logs - there is a meaningful time span between `start`
and `done` now and not just zeros all the time.
## Breaking Changes
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
faca9a3d5a6887517d02b994a43d0e1101b718bc test: Avoid intermittent issues due to async events in validationinterface_tests (MarcoFalke)
Pull request description:
Currently the tests have many issues:
* They setup the genesis block, even though it is not needed
* They queue an async `UpdatedBlockTip` even, which causes intermittent issues: https://github.com/bitcoin/bitcoin/issues/28146#issuecomment-1650064645
Fix all issues by trimming down the setup to just `ChainTestingSetup`.
ACKs for top commit:
Crypt-iQ:
tACK faca9a3d5a6887517d02b994a43d0e1101b718bc
Tree-SHA512: 4449040330f89bbaf5ce5b2052417c160b451c373987fdf1069596c07834ed81f0aea1506d53c7d2cd21062b27332d30679285dae194b272fd0cb9ce5ded32cf
7777f2a4bb1f9d843bc50a4e35085cfbb2808780 miner: Avoid stack-use-after-return in validationinterface (MarcoFalke)
fa5ceb25fce2200edf6b8ebfa6d4f01ed6774b95 test: Remove UninterruptibleSleep from test and replace it by SyncWithValidationInterfaceQueue (MarcoFalke)
fa770ce7fe67685c43780e219d8232efbee0bb8e validationinterface: Rework documentation, Rename pwalletIn to callbacks (MarcoFalke)
fab6d060ce5f580db538070beec1c5518c8c777c test: Add unregister_validation_interface_race test (MarcoFalke)
Pull request description:
When a validationinterface has itself unregistered in one thread, but is about to get executed in another thread [1], there is a race:
* The validationinterface destructing itself
* The validationinterface getting dereferenced for execution
[1] 64139803f1/src/validationinterface.cpp (L82-L83)
This happens in the miner. More generally it happens everywhere where at least one thread is generating notifications and another one is unregistering a validationinterface.
This issue has been fixed in commit ab31b9d6fe7b39713682e3f52d11238dbe042c16, but the fix has not been applied to the miner.
Example where this happened in practice: https://travis-ci.org/github/bitcoin/bitcoin/jobs/675322230#L4414
ACKs for top commit:
promag:
Code review ACK 7777f2a4bb1f9d843bc50a4e35085cfbb2808780.
laanwj:
Code review ACK 7777f2a4bb1f9d843bc50a4e35085cfbb2808780
Tree-SHA512: 8087119243c71ba18a823a63515f3730d127162625d8729024278b447af29e2ff206f4840ee3d90bf84f93a2c5ab73b76c7e7044c83aa93b5b51047a166ec3d3
## Issue being fixed or feature implemented
- The name `CB_V19_VERSION` is confusing because CbTx v2 was introduced
in v14, not v19
https://github.com/dashpay/dash/blob/master/doc/release-notes/dash/release-notes-0.14.0.md#dip0004---coinbase-payload-v2
- There are magic numbers instead of constants in some places
- `CheckCbTx` should check whatever the upper limit is, not
`CB_V20_VERSION` specifically
## What was done?
Turn CbTx versions into enum using self-describing names
## How Has This Been Tested?
Run tests
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
979271a5d9ff887cb2efb199feaf9602c9b2086d macdeploy: remove unused detached-sig-apply (fanquake)
Pull request description:
Signature application is now done with signapple.
8435d7f11a/contrib/guix/libexec/codesign.sh (L84-L85)
ACKs for top commit:
laanwj:
ACK 979271a5d9ff887cb2efb199feaf9602c9b2086d
gruve-p:
ACK 979271a5d9
achow101:
ACK 979271a5d9ff887cb2efb199feaf9602c9b2086d
hebasto:
ACK 979271a5d9ff887cb2efb199feaf9602c9b2086d, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: ab51a609d00cead4f33bcfc5b5ff1008ee02363ab1f4c4bf9544631069c237bfa92eac4dfa231bff8a1d702bda6cc92b4151361f74f58e77b595e0cb82a8391a
dc12f2e212dfacbe238cf68eb454b9ec71169bbc test: improve error msg on previous release tarball extraction failure (kdmukai)
7121fd8fa7de50ff67157f81f9e0f267b9795dbb test: self-sign previous release binaries for arm64 macOS (kdmukai)
Pull request description:
## The Problem
If you run `test/get_previous_releases.py -b` on an M1 or M2 mac, you'll get an unsigned v23.0 binary in the arm64 tarball. macOS [sets stricter requirements on ARM binaries](https://news.ycombinator.com/item?id=26996578) so the unsigned arm64 binary is apparently completely unusable without being signed/notarized(?).
This means that any test that depends on a previous release (e.g. `wallet_backwards_compatibility.py`) will fail because the v23.0 node cannot launch:
```
TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_framework.py", line 563, in start_nodes
node.wait_for_rpc_connection()
File "/Users/kdmukai/dev/bitcoin-core/test/functional/test_framework/test_node.py", line 231, in wait_for_rpc_connection
raise FailedToStartError(self._node_msg(
test_framework.test_node.FailedToStartError: [node 2] bitcoind exited with status -9 during initialization
```
This can also be confirmed by downloading bitcoin-23.0-arm64-apple-darwin.tar.gz (https://bitcoincore.org/bin/bitcoin-core-23.0/) and trying to run any of the binaries manually on an M1 or M2 mac.
## Solution in this PR
(UPDATED) Per @ hebasto, we can self-sign the arm64 binaries. This PR checks each binary in the previous release's "bin/" and verifies if the arm64 binary is signed. If not, attempt to self-sign and confirm success.
(note: an earlier version of this PR downloaded the x86_64 binary as a workaround but this approach has been discarded)
## Longer term solution
If possible, produce signed arm64 binaries in a future v23.x tarball?
Note that this same problem affects the new v24.0.1 arm64 tarball so perhaps a signed v24.x.x tarball would also be ideal?
That being said, this PR will check all current and future arm64 binaries and self-sign as needed, so perhaps we need not worry about pre-signing the tarball binaries. And I did test a version of `get_previous_releases.py` that includes the new v24.0.1 binaries and it successfully self-signed both v23.0 and v24.0.1, as expected.
## Further info:
Somewhat related to: https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1265164753
And @ fanquake noted on IRC that you can confirm which binaries are or are not signed via:
```
$ codesign -v -d bitcoin-qt
bitcoin-qt: code object is not signed at all
```
ACKs for top commit:
hebasto:
ACK dc12f2e212dfacbe238cf68eb454b9ec71169bbc
Tree-SHA512: 644895f8e97f5ffb3c4754c1db2c48abd77fa100c2058e3c896af04806596fc2b9c807a3f3a2add5be53301ad40ca2b8171585bd254e691f6eb38714d938396b
dba123167236a172d2d33861d58aa94a19729671 test: previous releases: add v23.0 (Sjors Provoost)
Pull request description:
Follows the same pattern as d8b705f1caeb3b4a6790cb26e4e5584ca791d965 (v22.0) and 8a57a06a5062dd8dfdefca4e404d0ddbd2a3da1d (v0.21.0).
Starting from v23.0 there is a separate macOS release for x86_64 and aarch64.
ACKs for top commit:
prusnak:
Approach ACK dba123167236a172d2d33861d58aa94a19729671
Tree-SHA512: 249aeddd5e80e163578581e5c8e9b6579f3694abc3d1fb68dddb7b42d75021ad85266688ec4a365a6631d82a65a19873aff7ba61c0ea59d21f8adbe4b772dc16
3d415215699e718b3f6eea6e3c9fb2948476f930 build: perform /Applications symlink generation in macdeployqtplus (fanquake)
dac693671928aa3fc304e6a802abfffb2f4ec8fd build: perform all .tiff copying in macdeployqtplus (fanquake)
Pull request description:
Rather than maintaining 2 different versions of the same code (`.tiff` copying and symlink generation), consolidate to just the Python code, and use it on macOS and Linux. Previously Linux would perform the 2 actions in the makefile, and then would still be running the `macdeployqtplus` script, so it makes sense to further consolidate deployment operations into the script.
Guix Build (on x86_64):
```bash
23343f04c426c7ff078afae4e600a7028970d4d86eed8b7834696d9e4d684151 guix-build-3d415215699e/output/arm64-apple-darwin/SHA256SUMS.part
c28b2a2e4888bf84369aa25804e2576347d5ab09416354ec8b95c76a9d38ff96 guix-build-3d415215699e/output/arm64-apple-darwin/bitcoin-3d415215699e-arm64-apple-darwin-unsigned.dmg
9a57077b2bd722a7d85d26b66cbce5abdb791985fe9d9d37e884c79ba8751e24 guix-build-3d415215699e/output/arm64-apple-darwin/bitcoin-3d415215699e-arm64-apple-darwin-unsigned.tar.gz
d2b06dc5b86541798ace41dab569849f7403e7ff9ec329bda671ec84e6fad549 guix-build-3d415215699e/output/arm64-apple-darwin/bitcoin-3d415215699e-arm64-apple-darwin.tar.gz
608e7d51a44ab9c5b28eb3703a0f4fe98b4adff22c77a5502786b84bd96cc188 guix-build-3d415215699e/output/dist-archive/bitcoin-3d415215699e.tar.gz
3e483705b1f9f1fb8f6afedc8ad0214a6cb00e77f766c0b03c42d56f410d4362 guix-build-3d415215699e/output/x86_64-apple-darwin/SHA256SUMS.part
9370e3e3b7d47b5a44e64554cf3b6d7e0671b072c08cd251eacc7ec72ce2b53f guix-build-3d415215699e/output/x86_64-apple-darwin/bitcoin-3d415215699e-x86_64-apple-darwin-unsigned.dmg
ad0f68682d78c311497669fc3d627138be37510215d259b5f0b686d93e7d83b7 guix-build-3d415215699e/output/x86_64-apple-darwin/bitcoin-3d415215699e-x86_64-apple-darwin-unsigned.tar.gz
e09dce4ff692ef66d1f4818083c1880bcf3a79c53112561d9e929bb6e5ffc011 guix-build-3d415215699e/output/x86_64-apple-darwin/bitcoin-3d415215699e-x86_64-apple-darwin.tar.gz
```
ACKs for top commit:
laanwj:
Re-ACK 3d415215699e718b3f6eea6e3c9fb2948476f930
Tree-SHA512: 80dd66a6e94c5b3e8823ccb57dcb08a8851a1e70a154b62385443f8d2d5ed5af900a0ac5003143959863586f1c7b90002fe6bff3ca5e37697253e051f69d7629
1513727e2b38800c694d1204cb454cc6fabc4937 build, qt: (Re-)sign package (Hennadii Stepanov)
c26a0a5af76bed9c2eb65f1a19725508c55299e8 build, qt: Align frameworks with macOS codesign tool requirements (Hennadii Stepanov)
Pull request description:
Fixes#22403
This PR follows Apple [docs](https://developer.apple.com/documentation/macos-release-notes/macos-big-sur-11_0_1-universal-apps-release-notes):
> - New in macOS 11 on Macs with Apple silicon, and starting in macOS Big Sur 11 beta 6, the operating system enforces that any executable must be signed before it’s allowed to run. There isn’t a specific identity requirement for this signature: a simple ad-hoc signature is sufficient...
> - ... If you use a custom workflow involving tools that modify a binary after linking (e.g. `strip` or `install_name_tool`) you might need to manually call `codesign` as an additional build phase to properly ad-hoc sign your binary. These new signatures are not bound to the specific machine that was used to build the executable, they can be verified on any other system and will be sufficient to comply with the new default code signing requirement on Macs with Apple silicon...
When building with system Qt frameworks (i.e., without depends), a new string has been added to the `make deploy` log on M1-based macOS:
```
% make deploy
...
+ Generating .DS_Store +
dist/Bitcoin-Qt.app: replacing existing signature
+ Preparing .dmg disk image +
...
```
This PR does not change build system behavior:
- when building with depends
- on Intel-based macOS
ACKs for top commit:
jarolrod:
ACK 1513727e2b38800c694d1204cb454cc6fabc4937
fanquake:
ACK 1513727e2b38800c694d1204cb454cc6fabc4937 - although didn't test on M1 hardware. Given the forced signing is scoped to only occur when running the deploy script on macOS, this doesn't interfere with our release signing.
Tree-SHA512: 3aa778fdd6ddb54f029f632f2fe52c2ae3bb197ba564cb776493aa5c3a655bd51d10ccbe6c007372d717e9b01fc4193dd5c29ea0bc7e069dcae7e991ae259f0c
0a5723beea9c909b437e8c3fa434506019c1198c macdeploy: cleanup .temp.dmg if present (fanquake)
ecffe8689dfbdc33deba8119376dcc8f208f0f72 macdeploy: remove qt4 related code (fanquake)
639f0642539c6b5ba9bc7b39bb8bb52752029bee macdeploy: select the plugins we need, rather than excluding those we don't (fanquake)
3d26b6b9e928e3cdc4b3d8d1f66ec7ed022b411b macdeploy: fix framework printing when passing -verbose (fanquake)
dca6c9032993f2bbf8047751d52f2a5c7ebd3ee4 macdeploy: remove unused plistlib import (fanquake)
Pull request description:
This includes [one followup](https://github.com/bitcoin/bitcoin/pull/20422#discussion_r534207899) and [one bug fix](3d26b6b9e9) from #20422, as well as some simplifications to the `macdeployqtplus` code.
ACKs for top commit:
hebasto:
ACK 0a5723beea9c909b437e8c3fa434506019c1198c, tested on macOS Big Sur 11.4 (20F71, x86_64) + Homebrew's Qt 5.15.2.
Tree-SHA512: cfad9505eacd32fe3a9d06eb13b2de0b6d2cad7b17778e90b503501cbf922e53d4e7f7f74952d1aed58410bdae9b0bb3248098583ef5b85689cb27d4dc06c029
## Issue being fixed or feature implemented
Should hopefully fix
https://github.com/dashpay/dash-dev-branches/actions/runs/6939402277/job/18876687119#5716 follow-up
## What was done?
`$GITHUB_REPOSITORY` is not available inside docker, pass it inside
## How Has This Been Tested?
## Breaking Changes
n/a
## Checklist:
- [x] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have added or updated relevant unit/integration/functional/e2e
tests
- [ ] I have made corresponding changes to the documentation
- [x] I have assigned this pull request to a milestone _(for repository
code-owners and collaborators only)_
