💬 achow101 commented on pull request "test: add test for specifying custom pidfile via `-pid`":
(https://github.com/bitcoin/bitcoin/pull/30724#issuecomment-2433468377)
ACK 04e4d52420a0e6bf40d4bd6fe1f31f66db9eab0a

✅ danielabrozzoni closed a pull request: "rest: Support transaction broadcast in REST interface"
(https://github.com/bitcoin/bitcoin/pull/31065)

💬 danielabrozzoni commented on pull request "rest: Support transaction broadcast in REST interface":
(https://github.com/bitcoin/bitcoin/pull/31065#issuecomment-2433474617)
Closing as it doesn't seem this is the way to go, thanks everyone for the feedback! :)

📝 achow101 unlocked a pull request: "validation: fix coins disappearing mid-package evaluation"
(https://github.com/bitcoin/bitcoin/pull/28251)
While we are evaluating a package, we split it into "subpackages" for evaluation (currently subpackages all have size 1 except the last one). If a subpackage has size 1, we may add a tx to mempool and call `LimitMempoolSize()`, which evicts transactions if the mempool gets full. We handle the case where the just-submitted transaction is evicted immediately, but we don't handle the case in which a transaction from a previous subpackage (either just submitted or already in mempool) is evicted. Mai
...

💬 sdaftuar commented on pull request "validation: fix coins disappearing mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#issuecomment-2433501732)
I'm reviewing this PR more carefully, after seeing @instagibbs' comment here: https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1813272637

As I understand it, I think there are two key behaviors introduced in this PR:
1) Clearing state between subpackage evaluations (specifically, clearing the coins in `m_viewmempool`)
2) Deferring mempool limiting until the end of package validation

However, the specific issue that @glozow described above (https://github.com/bitcoin/bitcoin/pull
...

💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2433506200)
> In [221410b](https://github.com/bitcoin/bitcoin/commit/221410bc14616eaf7017e5097cd6848fcb4f8e1b) "bench: specialize working directory name" , the commit message says
>
> > After this commit, benchmarks are contained within its own directory:
> > /<OS_tmp_dir>/test_common bitcoin/<benchmark_name>/<random_uint256>/
>
> However I am not observing this behavior. It omits the benchmark name.
>
> The benchmark name is included when `-testdatadir` is used.

Yeah, that's because the data d
...

🚀 achow101 merged a pull request: "test: add test for specifying custom pidfile via `-pid`"
(https://github.com/bitcoin/bitcoin/pull/30724)

💬 achow101 commented on pull request "init: Some small chainstate load improvements":
(https://github.com/bitcoin/bitcoin/pull/31046#issuecomment-2433519694)
ACK 31cc5006c3de4dd6a1f7a238684163956604df45

🤔 furszy reviewed a pull request: "bench: add support for custom data directory"
(https://github.com/bitcoin/bitcoin/pull/31000#pullrequestreview-2390711656)
Updated per feedback. Thanks achow.

💬 achow101 commented on pull request "doc: Extend developer-notes with file-name-only debugging fix":
(https://github.com/bitcoin/bitcoin/pull/30670#issuecomment-2433523846)
ACK d1546b3c4f24d3ff6abe729a4288e827786cf6e9

👍 theStack approved a pull request: "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`"
(https://github.com/bitcoin/bitcoin/pull/29954#pullrequestreview-2390749484)
ACK modulo nits

As others have expressed before, I'd also slightly prefer to name the options exactly like their command line option (i.e. "datacarriersize" without the "max" prefix), but no blocker.

💬 theStack commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1813770793)
nit, a little shorter:
```suggestion
ret.pushKV("maxdatacarriersize", pool.m_opts.max_datacarrier_bytes.value_or(0));
```

💬 theStack commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1813775127)
```suggestion
from test_framework.util import (
assert_equal,
assert_raises_rpc_error,
)
```
(we prefer multi-line imports, see style guidelines in ./test/functional/README.md)

💬 achow101 commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1813825732)
In ad0a8ffceacfd5ce37826b2882b0e51a14c5a6bf "test: compare BDB dumps of test framework parser and wallet tool"

Since we generate the large labels, this could also check that all show up with the correct address.

💬 achow101 commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1813809067)
In ad0a8ffceacfd5ce37826b2882b0e51a14c5a6bf "test: compare BDB dumps of test framework parser and wallet tool"

Could use `self.assert_tool_output`, see how the other calls to `dump` are done in this test.

💬 achow101 commented on pull request "test: improve BDB parser (handle internal/overflow pages, support all page sizes)":
(https://github.com/bitcoin/bitcoin/pull/30125#discussion_r1813830674)
In ad0a8ffceacfd5ce37826b2882b0e51a14c5a6bf "test: compare BDB dumps of test framework parser and wallet tool"

In addition to testing overflow pages, we can also test internal pages by generating a bunch of keys with `keypoolrefill`.

🚀 achow101 merged a pull request: "init: Some small chainstate load improvements"
(https://github.com/bitcoin/bitcoin/pull/31046)

💬 achow101 commented on pull request "test: Fix intermittent issue in wallet_backwards_compatibility.py":
(https://github.com/bitcoin/bitcoin/pull/29982#issuecomment-2433618589)
ACK ec777917d6eba0b417dbc90b9b891240a44b7ec4

lgtm for a mitigation.

💬 kevkevinpal commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1813849800)
thanks! fair point updated in [4bdb78c](https://github.com/bitcoin/bitcoin/pull/31139/commits/4bdb78cfe76414227b749674ed660f4972fb6eda)

💬 jarolrod commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2433812861)
concept ack