Bitcoin Core Github
44 subscribers
119K links
Download Telegram
🚀 fanquake merged a pull request: "build: add root dir to CMAKE_PREFIX_PATH in toolchain"
(https://github.com/bitcoin/bitcoin/pull/32798)
💬 hebasto commented on pull request "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)":
(https://github.com/bitcoin/bitcoin/pull/32564#issuecomment-3004291231)
@hodlinator

Thank you for the review. Your feedback has been addressed.
📝 fanquake opened a pull request: "[29.x] More backports"
(https://github.com/bitcoin/bitcoin/pull/32810)
Backports:
* #32798
💬 fanquake commented on pull request "build: add root dir to CMAKE_PREFIX_PATH in toolchain":
(https://github.com/bitcoin/bitcoin/pull/32798#issuecomment-3004302416)
Backported to 29.x in #32810.
📝 fanquake opened a pull request: "[28.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32811)
Backports:
* #32765
* #32776
* #32777
🤔 maflcko reviewed a pull request: "test: headers sync timeout"
(https://github.com/bitcoin/bitcoin/pull/32677#pullrequestreview-2957517215)
lgtm. Nice test for both branches

review ACK 8d257ed5937a47e1a60b7fe085f2e984eb60a956 👋

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
tru
...
💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166328136)

getheader -> getheaders [matches the method and plural usage elsewhere]
💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166409317)
```suggestion
with node.assert_debug_log(expected_msgs=["initial getheaders (0) to peer=0"]):
peer1 = node.add_p2p_connection(P2PInterface())
peer1.wait_for_getheaders()
```

nit: it is a bit better to check the state (and wait for it directly) than to indirectly read the debug log in a loop until a specific log line is hit.
💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166390209)
```suggestion
variable_timeout_sec = math.ceil(HEADERS_DOWNLOAD_TIMEOUT_PER_HEADER_MS /1_000 *
seconds_since_best_header / POW_TARGET_SPACING_SEC) # ceil, to make this function usable with mocktime, which only has second precision.

return (current_time + HEADERS_DOWNLOAD_TIMEOUT_BASE_SEC + variable_timeout_sec)
```

seems a bit odd to be rounding down in this function and then apply a random +1 offset below. So it is actually rounding up. It would be
...
💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166420099)
also, could check the getheaders `block_hash=int(node.getbestblockhash(), 16)`
💬 maflcko commented on pull request "test: headers sync timeout":
(https://github.com/bitcoin/bitcoin/pull/32677#discussion_r2166403740)
nit: it is probably a bit faster to `self.nodes[0].disconnect_p2ps()` when the goal is to disconnect all peers (and throw away their peer state)
🤔 dergoegge requested changes to a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2957730107)
Code review ACK 2ac8696b53e455dd27c8341828404a23b5cb68a9

I think the commit order needs to change, so that 3c30ee9107fd0e916f9784b091a4d02f3a73ce46 comes before the tests that make use of the change?

nit: I'd also suggest to squash b44d31455ad46ca3ed95690dfc0a913445b9c1c9 and 5fe08e01384c4a4c8525060d365fa38c312758f0 into one commit, so there is no intermediate state between the commits where the validation logs are rate limited.
💬 josibake commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3004472779)
> or is this issue MULTIPROCESS=1 capnp specific?

I don't think the issue is specific to multiprocess, but rather I'm running into the issue when trying to build multiprocess. I think the root cause is as @vasild mentions, i.e., hardcoding gcc/g++. Admittedly, it _is_ weird that I'm hitting this with multiprocess but its because I am specifically trying to build in an environment without gcc/g++.

I was surprised that `make -C depends -j$(nproc) CC=clang CXX=clang++ MULTIPROCESS=1` didn't w
...
🤔 hodlinator reviewed a pull request: "miniscript, refactor: Make `operator""_mst` `consteval` (re-take)"
(https://github.com/bitcoin/bitcoin/pull/32564#pullrequestreview-2957841876)
re-ACK a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf
💬 instagibbs commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3004501987)
@glozow this sounds right. I wonder if we'd want to have LimitOrphans deterministically called inside `AddTx/AddAnnouncer` such that we can assert what the max amount is being trimmed, in count/size for coverage?
💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166253286)
Would be good to verify the value as well:
```suggestion
assert_equal(raw['sigopsize'], <calculation>)
```
💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166299222)
Missing punctuation (also found by Drahtbot LLM):
```suggestion
**BIP‑141 virtual size** (sometimes referred to as `vsize_bip141`) is simply the block weight divided by four, rounded up.
```
💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166355936)
Q1: Why do we want to only conditionally report `sigopsize`, is there some comment to that affect from the old PR? I don't get that impression from https://github.com/bitcoin/bitcoin/pull/27591#discussion_r1376980882.

Q2: In the above linked comment, do you understand what is meant by "(Or, even better, do the sigop adjustment in cases where we can -- getrawtransaction of txs in blocks could pull up the utxo being spent via rev*.dat and calculate the sigop adjustment?)"? I must admit I don't
...
💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166305252)
nit: Would suggest wrapping the line width at 80 or 100 if you want to wrap it, or something closer to what other docs do in this folder.
💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2166360506)
nit: https://github.com/bitcoin/bitcoin/pull/27591#discussion_r1378636536 mentions adding `sigopsize` to other RPCs as well. Not sure if necessary.