Bitcoin Core Github
44 subscribers
121K links
Download Telegram
๐Ÿ“ ryanofsky opened a pull request: "Update libmultiprocess library"
(https://github.com/bitcoin/bitcoin/pull/31105)
Add recent changes and fixes for shutdown bugs.

https://github.com/chaincodelabs/libmultiprocess/pull/111: doc: Add internal design section
https://github.com/chaincodelabs/libmultiprocess/pull/113: Add missing include to util.h
https://github.com/chaincodelabs/libmultiprocess/pull/116: shutdown bugfix: destroy RPC system before running cleanup callbacks
https://github.com/chaincodelabs/libmultiprocess/pull/118: shutdown bugfix: Prevent segfault in server if connection is broken during lon
...
๐Ÿ‘ BrandonOdiwuor approved a pull request: "doc: remove dependency install instructions from win docs"
(https://github.com/bitcoin/bitcoin/pull/31100#pullrequestreview-2373066201)
ACK 184f12c1542f6c53eb2bd9dfb08dfdd490e38846
๐Ÿค” l0rinc reviewed a pull request: "Don't wipe coins cache when full and instead evict LRU clean entries"
(https://github.com/bitcoin/bitcoin/pull/31102#pullrequestreview-2373049420)
Since this depends on other PRs, would it make sense to draft it until those are merged and CI fixed?
๐Ÿ’ฌ l0rinc commented on pull request "Don't wipe coins cache when full and instead evict LRU clean entries":
(https://github.com/bitcoin/bitcoin/pull/31102#discussion_r1803458000)
it seemed to me that often we're only only checking the memory after we're finished with certain batches.
E.g starting `bitcoind` with `-dbcache=4` was still chugging along at 200k with `cache=128.7MiB`.
Would it be safer to lower this to 95%?
๐Ÿ’ฌ l0rinc commented on pull request "Don't wipe coins cache when full and instead evict LRU clean entries":
(https://github.com/bitcoin/bitcoin/pull/31102#discussion_r1803459850)
formatting is really confusing here
๐Ÿ’ฌ l0rinc commented on pull request "Don't wipe coins cache when full and instead evict LRU clean entries":
(https://github.com/bitcoin/bitcoin/pull/31102#discussion_r1803460218)
this is very cheap, can we make it an assert instead?
๐Ÿ’ฌ l0rinc commented on pull request "Don't wipe coins cache when full and instead evict LRU clean entries":
(https://github.com/bitcoin/bitcoin/pull/31102#discussion_r1803462933)
same, formatting is inconsistent with `AddFlags` (where AddToLinkedList after the `if` is inside the condition, but here it's outside)
๐Ÿ’ฌ l0rinc commented on pull request "Don't wipe coins cache when full and instead evict LRU clean entries":
(https://github.com/bitcoin/bitcoin/pull/31102#discussion_r1803449057)
nit: can we unify the formatting here?
๐Ÿ’ฌ l0rinc commented on pull request "Don't wipe coins cache when full and instead evict LRU clean entries":
(https://github.com/bitcoin/bitcoin/pull/31102#discussion_r1803460880)
`linked list` is an implementation detail, can we rather say the purpose of inclusion instead/
๐Ÿ’ฌ l0rinc commented on pull request "Don't wipe coins cache when full and instead evict LRU clean entries":
(https://github.com/bitcoin/bitcoin/pull/31102#discussion_r1803466111)
could you please fix this in the commit that introduced the error instead?
๐Ÿ’ฌ l0rinc commented on pull request "Don't wipe coins cache when full and instead evict LRU clean entries":
(https://github.com/bitcoin/bitcoin/pull/31102#discussion_r1803468957)
the doc states this tries to evict entries - would it make sense to mention that instead of the memory cleaning effect of it?
๐Ÿ’ฌ TheBlueMatt commented on issue "Stratum v2 via IPC Mining Interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2417419737)
> The mining interface in interfaces/mining.h, node/interfaces.cpp and mining.capnp is orders of magnitude less code than what's currently in https://github.com/Sjors/bitcoin/pull/68. That would be still be true after a refactor of Sv2Connman.

I'm curious where the bulk of that is. It should be basically identical in terms of total volume (framing and such all has to happen either way, even if its auto-generated code when using capnproto), the protocol would be ~identical in function, and IPC
...
๐Ÿ’ฌ TheBlueMatt commented on issue "Stratum v2 via IPC Mining Interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2417423369)
Its probably worth pointing out that with new MEV-extraction things coming out these days Bitcoin Core will not be the only server of work, which means we also need to define new mining work-providing protocols in terms of the server-side behavior as well (not just "whatever Bitcoin Core does", and not just whatever goes in its docs).
๐Ÿ’ฌ adamandrews1 commented on pull request "fees: document non-monotonic estimation edge case":
(https://github.com/bitcoin/bitcoin/pull/31080#issuecomment-2417765473)
Concept ACK
Approach ACK
๐Ÿ’ฌ 1440000bytes commented on pull request "rest: Support transaction broadcast in REST interface":
(https://github.com/bitcoin/bitcoin/pull/31065#issuecomment-2417770876)
By default, the RPC interface can only be accessed [locally](https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#security). However, a user may want to keep the RPC local and enable the REST interface with [`rest=1`](url) to allow public access to available [endpoints](https://github.com/bitcoin/bitcoin/blob/master/doc/REST-interface.md).

This pull request adds a basic endpoint to the REST interface for broadcasting transactions. As long as Bitcoin Core supports the REST
...
๐Ÿ‘ pablomartin4btc approved a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2373584648)
re-ACK 15cfeebb47587af6ce7be72fd52c57a0483d86d2

Changes since my last review: removing the `-testdatadir` arg from `bench-bitcoin` and reusing it from the utils `setup_common`.
๐Ÿค” ariard reviewed a pull request: "validation: Improve input script check error reporting"
(https://github.com/bitcoin/bitcoin/pull/31097#pullrequestreview-2373597013)
utACK ad51775
๐Ÿ’ฌ ariard commented on pull request "validation: Improve input script check error reporting":
(https://github.com/bitcoin/bitcoin/pull/31097#discussion_r1803751039)
I was reading the comment just above this one i.e โ€œCheck whether the failure was caused by a non-mandatory script verification check, such as non-standard DER encodings or non-null dummy argumentsโ€.

I think the 2 examples given in this comment are actually โ€œmandatory-script-verify-flagโ€, i.e CONSENSUS and not NOT_STANDARD:
- DER encodings (BIP66 and `SCRIPT_VERIFY_DERSIG`), interpreter flag is set in `GetBlockScriptFlags` https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L237
...
๐Ÿ’ฌ ariard commented on pull request "validation: Improve input script check error reporting":
(https://github.com/bitcoin/bitcoin/pull/31097#discussion_r1803776359)
I think we should have further tests that a non-standard transaction which is consensus valid before an activation height, is considered as consensus invalid after the activation height.

E.g on regtest, the deployment height are always active unless overridden. I donโ€™t think we do that in invalid_txs.py` currently: https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_invalid_tx.py#L28
๐Ÿ‘ adamandrews1 approved a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2373745550)
ACK