💬 rkrux commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2560222785)
I think the test cases can be greatly simplified with the following diff:
```diff
diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
index 49c41b0a1f..588d7725dc 100755
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -25,6 +25,7 @@ from test_framework.descriptors import descsum_create
from test_framework.util import (
assert_equal,
assert_raises_rpc_error,
+ assert_greater_th
...
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2560222785)
I think the test cases can be greatly simplified with the following diff:
```diff
diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py
index 49c41b0a1f..588d7725dc 100755
--- a/test/functional/wallet_importdescriptors.py
+++ b/test/functional/wallet_importdescriptors.py
@@ -25,6 +25,7 @@ from test_framework.descriptors import descsum_create
from test_framework.util import (
assert_equal,
assert_raises_rpc_error,
+ assert_greater_th
...
💬 rkrux commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2560225153)
Would be good to add a small test case for this newly added condition as well.
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r2560225153)
Would be good to add a small test case for this newly added condition as well.
💬 yuvicc commented on pull request "test: Fix reorg patterns in tests to use proper fork-based approach":
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3576089382)
Rebased.
(https://github.com/bitcoin/bitcoin/pull/32587#issuecomment-3576089382)
Rebased.
📝 stickies-v opened a pull request: "kernel: don't use assert to handle invalid user input"
(https://github.com/bitcoin/bitcoin/pull/33943)
Crashing with `assert` for invalid user input is problematic for a library, e.g. this is impossible for FFI users to recover from, forcing them to re-implement the error handling logic client-side. See e.g. the [go](https://github.com/stringintech/go-bitcoinkernel/blob/88105c69b32c48919a45bb07fc05ca2c79d2728b/kernel/script_pubkey.go#L91-L102) and [.net](https://github.com/janb84/BitcoinKernel.NET/blob/4ba1ff802fe9a3b7c0af0012e6fdc33e5bc4f961/src/BitcoinKernel.Core/ScriptVerification/ScriptVerifi
...
(https://github.com/bitcoin/bitcoin/pull/33943)
Crashing with `assert` for invalid user input is problematic for a library, e.g. this is impossible for FFI users to recover from, forcing them to re-implement the error handling logic client-side. See e.g. the [go](https://github.com/stringintech/go-bitcoinkernel/blob/88105c69b32c48919a45bb07fc05ca2c79d2728b/kernel/script_pubkey.go#L91-L102) and [.net](https://github.com/janb84/BitcoinKernel.NET/blob/4ba1ff802fe9a3b7c0af0012e6fdc33e5bc4f961/src/BitcoinKernel.Core/ScriptVerification/ScriptVerifi
...
💬 instagibbs commented on pull request "txgraph: drop move assignment operator":
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3576309301)
ACK ade0397f59f2fb59ab0e4ebb39869ac343cc54ee
(https://github.com/bitcoin/bitcoin/pull/33862#issuecomment-3576309301)
ACK ade0397f59f2fb59ab0e4ebb39869ac343cc54ee
💬 fanquake commented on pull request "doc: document capnproto and libmultiprocess deps in 29.x":
(https://github.com/bitcoin/bitcoin/pull/33623#issuecomment-3576316460)
Concept ACK - @willcl-ark did you want to address any of the suggestions here?
(https://github.com/bitcoin/bitcoin/pull/33623#issuecomment-3576316460)
Concept ACK - @willcl-ark did you want to address any of the suggestions here?
💬 lucasbalieiro commented on issue "Memory leak when using IPC mining interface":
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3576336813)
here are some insights from the experiments I’ve been running over the past few days.
When I first reported this issue (https://github.com/stratum-mining/sv2-apps/pull/59#issuecomment-3568252007], my server was also running a few utility tools that I typically use during long test sessions.
To eliminate the “maybe it’s something else eating the memory” hypothesis, I started a fresh session with **only** the following running:
* `sv2-apps`
* Bitcoin Core v30 (mainnet)
* The essential system pr
...
(https://github.com/bitcoin/bitcoin/issues/33940#issuecomment-3576336813)
here are some insights from the experiments I’ve been running over the past few days.
When I first reported this issue (https://github.com/stratum-mining/sv2-apps/pull/59#issuecomment-3568252007], my server was also running a few utility tools that I typically use during long test sessions.
To eliminate the “maybe it’s something else eating the memory” hypothesis, I started a fresh session with **only** the following running:
* `sv2-apps`
* Bitcoin Core v30 (mainnet)
* The essential system pr
...
👍 instagibbs approved a pull request: "test: Fix reorg patterns in tests to use proper fork-based approach"
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-3505760522)
reACK 2a64851c34fcf9955a673eb77463f68ab583a7bd
`git range-diff master b1581924a79e120f166719943646484ba00ff21b 2a64851c34fcf9955a673eb77463f68ab583a7bd`
(https://github.com/bitcoin/bitcoin/pull/32587#pullrequestreview-3505760522)
reACK 2a64851c34fcf9955a673eb77463f68ab583a7bd
`git range-diff master b1581924a79e120f166719943646484ba00ff21b 2a64851c34fcf9955a673eb77463f68ab583a7bd`
💬 hebasto commented on pull request "test: Introduce `SUPPRESS_ABORT_MESSAGE` environment variable":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-3576356937)
> > @maflcko
> > > No objection, but assert/Assert seems to be used widely in the codebase, so shouldn't this be done for all test binaries, or none? Otherwise the same assert could lead to inconsistent behavior, depending on which test binary ran into it.
> >
> >
> > Thanks! Reworked.
>
> Thx, but it seems it would also need to be applied to bitcoind (or really all executables)?
It is already implemented for `bitcoind.exe` here:https://github.com/bitcoin/bitcoin/blob/5336bcd5784925c
...
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-3576356937)
> > @maflcko
> > > No objection, but assert/Assert seems to be used widely in the codebase, so shouldn't this be done for all test binaries, or none? Otherwise the same assert could lead to inconsistent behavior, depending on which test binary ran into it.
> >
> >
> > Thanks! Reworked.
>
> Thx, but it seems it would also need to be applied to bitcoind (or really all executables)?
It is already implemented for `bitcoind.exe` here:https://github.com/bitcoin/bitcoin/blob/5336bcd5784925c
...
💬 Sjors commented on pull request "mining: add getMemoryLoad() and track template non-mempool memory footprint":
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3576359913)
I restructured the implementation and commits a bit.
The `TxTemplateMap` now lives on the `NodeContext` rather than `MinerImpl` (interface). This reflects the fact that we want to track the global memory footprint instead of per client. It's a lightweight member `template_tx_refs` which should be easy to fold into a block template manager later.
It also made it easier to move `GetTemplateMemoryUsage` from `interface.cpp` to `miner.cpp`, where it's more reusable.
This in turn let me sp
...
(https://github.com/bitcoin/bitcoin/pull/33922#issuecomment-3576359913)
I restructured the implementation and commits a bit.
The `TxTemplateMap` now lives on the `NodeContext` rather than `MinerImpl` (interface). This reflects the fact that we want to track the global memory footprint instead of per client. It's a lightweight member `template_tx_refs` which should be easy to fold into a block template manager later.
It also made it easier to move `GetTemplateMemoryUsage` from `interface.cpp` to `miner.cpp`, where it's more reusable.
This in turn let me sp
...
🚀 fanquake merged a pull request: "txgraph: drop move assignment operator"
(https://github.com/bitcoin/bitcoin/pull/33862)
(https://github.com/bitcoin/bitcoin/pull/33862)
📝 instagibbs converted_to_draft a pull request: "policy: don't CheckEphemeralSpends on reorg"
(https://github.com/bitcoin/bitcoin/pull/33616)
Similar reasoning to https://github.com/bitcoin/bitcoin/pull/33504
During a deeper reorg it's possible that a long sequence of dust-having transactions that are connected in a linear fashion. On reorg, this could cause each subsequent "generation" to be rejected. These rejected transactions may contain a large amount of competitive fees via normal means.
PreCheck based `PreCheckEphemeralSpends` is left in place because we wouldn't have relayed them prior to the reorg.
based on #32587 to
...
(https://github.com/bitcoin/bitcoin/pull/33616)
Similar reasoning to https://github.com/bitcoin/bitcoin/pull/33504
During a deeper reorg it's possible that a long sequence of dust-having transactions that are connected in a linear fashion. On reorg, this could cause each subsequent "generation" to be rejected. These rejected transactions may contain a large amount of competitive fees via normal means.
PreCheck based `PreCheckEphemeralSpends` is left in place because we wouldn't have relayed them prior to the reorg.
based on #32587 to
...
💬 instagibbs commented on pull request "policy: don't CheckEphemeralSpends on reorg":
(https://github.com/bitcoin/bitcoin/pull/33616#issuecomment-3576395944)
marking as draft for now until it becomes dependency-less and I've looked over the tests some more
(https://github.com/bitcoin/bitcoin/pull/33616#issuecomment-3576395944)
marking as draft for now until it becomes dependency-less and I've looked over the tests some more
👋 instagibbs's pull request is ready for review: "policy: Remove individual transaction <minrelay restriction"
(https://github.com/bitcoin/bitcoin/pull/33892)
(https://github.com/bitcoin/bitcoin/pull/33892)
📝 oxyzenQ opened a pull request: "Request: fix typos in "release-notes-0.18.0.md" and "release-notes-0.15.0.1.md""
(https://github.com/bitcoin/bitcoin/pull/33944)
Typos Identified:
1. "seperate" → should be "separate"
- File: doc/release-notes/release-notes-0.18.0.md
- Line: 909-947 region
- Text: "#15043 Build fuzz targets into seperate executables (MarcoFalke)"
- URL: https://github.com/bitcoin/bitcoin/blob/0690514d4f72aac251ee0b876cded9187d42c63e/doc/release-notes/release-notes-0.18.0.md#L909-L947
2. "occuring" → should be "occurring"
- File: doc/release-notes/release-notes-0.15.0.1.md
- Line: 1-87 region
- Text: "This is a minimal patch t
...
(https://github.com/bitcoin/bitcoin/pull/33944)
Typos Identified:
1. "seperate" → should be "separate"
- File: doc/release-notes/release-notes-0.18.0.md
- Line: 909-947 region
- Text: "#15043 Build fuzz targets into seperate executables (MarcoFalke)"
- URL: https://github.com/bitcoin/bitcoin/blob/0690514d4f72aac251ee0b876cded9187d42c63e/doc/release-notes/release-notes-0.18.0.md#L909-L947
2. "occuring" → should be "occurring"
- File: doc/release-notes/release-notes-0.15.0.1.md
- Line: 1-87 region
- Text: "This is a minimal patch t
...
💬 fanquake commented on pull request "contrib: Count entry differences in asmap-tool diff summary":
(https://github.com/bitcoin/bitcoin/pull/33939#issuecomment-3576475295)
cc @hodlinator
(https://github.com/bitcoin/bitcoin/pull/33939#issuecomment-3576475295)
cc @hodlinator
🤔 ismaelsadeeq reviewed a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3505882668)
reACK c34bc01b2ff2fc91ed4020288c5fa15f0c5b075e
I think recommending to fuzz using docker is a good idea.
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3505882668)
reACK c34bc01b2ff2fc91ed4020288c5fa15f0c5b075e
I think recommending to fuzz using docker is a good idea.
✅ fanquake closed an issue: "RFC: Do we want to support fuzzing on MacOS?"
(https://github.com/bitcoin/bitcoin/issues/33731)
(https://github.com/bitcoin/bitcoin/issues/33731)
🚀 fanquake merged a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921)
(https://github.com/bitcoin/bitcoin/pull/33921)
💬 ismaelsadeeq commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2560644830)
I see your point @l0rinc but IHMO that should be in a separate PR? Tested independently and reviewed by contributors ensuring the guide works as expected.
But prior to that we can merge this, and clear the ambiguity.
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2560644830)
I see your point @l0rinc but IHMO that should be in a separate PR? Tested independently and reviewed by contributors ensuring the guide works as expected.
But prior to that we can merge this, and clear the ambiguity.