💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426849181)
nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Replace "disk" with "storage" or "persistent storage", so that non-spinning storage doesn't feel excluded?
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426849181)
nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Replace "disk" with "storage" or "persistent storage", so that non-spinning storage doesn't feel excluded?
💬 maflcko commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426853766)
nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Replace `bitcoind` with "the program" or "Bitcoin Core", so that `bitcoin-node` or `bitcoin-qt` do not feel excluded?
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1426853766)
nit in https://github.com/bitcoin/bitcoin/commit/db5ed8682f99c64a97e598bda69147307b447820: Replace `bitcoind` with "the program" or "Bitcoin Core", so that `bitcoin-node` or `bitcoin-qt` do not feel excluded?
💬 vasild commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426859372)
Or replace `UniValue` with https://github.com/nlohmann/json
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426859372)
Or replace `UniValue` with https://github.com/nlohmann/json
💬 sipa commented on pull request "fuzz: Improve fuzzing stability for minisketch harness":
(https://github.com/bitcoin/bitcoin/pull/29064#issuecomment-1856042019)
This fixes it I think:
```diff
--- a/src/test/fuzz/minisketch.cpp
+++ b/src/test/fuzz/minisketch.cpp
@@ -65,7 +65,9 @@ FUZZ_TARGET(minisketch)
sketch_br.Deserialize(sketch_b.Serialize());
Minisketch sketch_diff{std::move(fuzzed_data_provider.ConsumeBool() ? sketch_a : sketch_ar)};
- sketch_diff.Merge(fuzzed_data_provider.ConsumeBool() ? sketch_b : sketch_br);
+ auto& sketch_merge = fuzzed_data_provider.ConsumeBool() ? sketch_b : sketch_br;
+ if (sketch_diff.GetIm
...
(https://github.com/bitcoin/bitcoin/pull/29064#issuecomment-1856042019)
This fixes it I think:
```diff
--- a/src/test/fuzz/minisketch.cpp
+++ b/src/test/fuzz/minisketch.cpp
@@ -65,7 +65,9 @@ FUZZ_TARGET(minisketch)
sketch_br.Deserialize(sketch_b.Serialize());
Minisketch sketch_diff{std::move(fuzzed_data_provider.ConsumeBool() ? sketch_a : sketch_ar)};
- sketch_diff.Merge(fuzzed_data_provider.ConsumeBool() ? sketch_b : sketch_br);
+ auto& sketch_merge = fuzzed_data_provider.ConsumeBool() ? sketch_b : sketch_br;
+ if (sketch_diff.GetIm
...
💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1856045015)
I renamed `-stratumv2` to `-sv2` and `stratumv2port` to `-sv2port`. Added `-sv2interval` (default 30 seconds) and `-sv2feedelta` (default 1000 sat) to control how often templates are updated.
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1856045015)
I renamed `-stratumv2` to `-sv2` and `stratumv2port` to `-sv2port`. Added `-sv2interval` (default 30 seconds) and `-sv2feedelta` (default 1000 sat) to control how often templates are updated.
💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426869879)
> In commit "ArgsManager: return path by value from GetBlocksDirPath()" (https://github.com/bitcoin/bitcoin/commit/856c88776f8486446602476a1c9e133ac0cff510)
Wrong commit? :)
> documention
Thanks, copy-pasted this and removed my one-line comment.
(https://github.com/bitcoin/bitcoin/pull/29040#discussion_r1426869879)
> In commit "ArgsManager: return path by value from GetBlocksDirPath()" (https://github.com/bitcoin/bitcoin/commit/856c88776f8486446602476a1c9e133ac0cff510)
Wrong commit? :)
> documention
Thanks, copy-pasted this and removed my one-line comment.
💬 maflcko commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1856053272)
> To verify that nothing has been forgotten in the last commit which renames `fs::path::u8string()` to `fs::path::utf8string()` I added `std::u8string u8string() const = delete;`
This should not be needed, because the two functions return different and incompatible types, so any oversight should result in a compile failure as-is.
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1856053272)
> To verify that nothing has been forgotten in the last commit which renames `fs::path::u8string()` to `fs::path::utf8string()` I added `std::u8string u8string() const = delete;`
This should not be needed, because the two functions return different and incompatible types, so any oversight should result in a compile failure as-is.
💬 sipa commented on pull request "refactor: Remove pre-C++20 code, fs::path cleanup":
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1856056462)
@vasild FWIW, Bitcoin Core used to use json-spirit before the introduction of UniValue. The motivating argument was being able to access the string representation of numeric value (because when amounts are passed as JSON "numbers", we don't want to roundtrip through `double` before converting to `CAmount`).
(https://github.com/bitcoin/bitcoin/pull/29040#issuecomment-1856056462)
@vasild FWIW, Bitcoin Core used to use json-spirit before the introduction of UniValue. The motivating argument was being able to access the string representation of numeric value (because when amounts are passed as JSON "numbers", we don't want to roundtrip through `double` before converting to `CAmount`).
💬 hebasto commented on pull request "ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1856059883)
> I still don't really understand this fix
This fix makes Homebrew skip upgrading the `aws-sam-cli` package that we do not use.
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1856059883)
> I still don't really understand this fix
This fix makes Homebrew skip upgrading the `aws-sam-cli` package that we do not use.
💬 maflcko commented on issue "bitcoin-tx replaceable value should be optional, but isn't":
(https://github.com/bitcoin/bitcoin/issues/28638#issuecomment-1856061889)
See https://github.com/bitcoin/bitcoin/pull/29022
(https://github.com/bitcoin/bitcoin/issues/28638#issuecomment-1856061889)
See https://github.com/bitcoin/bitcoin/pull/29022
👍 instagibbs approved a pull request: "Make bitcoin-tx replaceable value optional"
(https://github.com/bitcoin/bitcoin/pull/29022#pullrequestreview-1782087346)
untested ACK https://github.com/bitcoin/bitcoin/pull/29022/commits/98afe7866185ed4157ffc581763e11dc02fcbae0
Test coverage looks good, thanks!
(https://github.com/bitcoin/bitcoin/pull/29022#pullrequestreview-1782087346)
untested ACK https://github.com/bitcoin/bitcoin/pull/29022/commits/98afe7866185ed4157ffc581763e11dc02fcbae0
Test coverage looks good, thanks!
👍 ryanofsky approved a pull request: "refactor: Remove pre-C++20 code, fs::path cleanup"
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1782116404)
Code review ACK 66667130416b86208e01a0eb5541a15ea805ac26. Just documentation change since last review.
To clarify and clean things up more, we could also document the `fs::path::utf8string()` method and `fs::u8path` function as being inverses of each other, and maybe rename `fs::u8path` to `fs::utf8path` for consistency. I don't think either of these things are necessary though, just thoughts for improvement.
(https://github.com/bitcoin/bitcoin/pull/29040#pullrequestreview-1782116404)
Code review ACK 66667130416b86208e01a0eb5541a15ea805ac26. Just documentation change since last review.
To clarify and clean things up more, we could also document the `fs::path::utf8string()` method and `fs::u8path` function as being inverses of each other, and maybe rename `fs::u8path` to `fs::utf8path` for consistency. I don't think either of these things are necessary though, just thoughts for improvement.
💬 theuni commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1856117820)
> > This one could be fixed alternatively (in theory) by nuking signals2, but I don't think this will happen any time soon.
>
> I know a [WIP] branch for this "exists" in some form. cc @theuni
It's not a WIP so much as a POC: https://github.com/theuni/bitcoin/commits/replace-boost-signals/
That implements the subset of signals that we rely on as a drop-in replacement. It passes tests and would serve as a good guide, but I don't think that's how we'd want to do it.
I could look at pic
...
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1856117820)
> > This one could be fixed alternatively (in theory) by nuking signals2, but I don't think this will happen any time soon.
>
> I know a [WIP] branch for this "exists" in some form. cc @theuni
It's not a WIP so much as a POC: https://github.com/theuni/bitcoin/commits/replace-boost-signals/
That implements the subset of signals that we rely on as a drop-in replacement. It passes tests and would serve as a good guide, but I don't think that's how we'd want to do it.
I could look at pic
...
💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426925744)
this block is worth encapsulating and adding unit tests to ensure the populated sets `ws.m_num_in_package_ancestors` and `ws.m_ancestors_of_in_package_ancestors` match expected
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426925744)
this block is worth encapsulating and adding unit tests to ensure the populated sets `ws.m_num_in_package_ancestors` and `ws.m_ancestors_of_in_package_ancestors` match expected
💬 jamesob commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1856138054)
Big concept ACK on the interface. This is a nice simplification.
(https://github.com/bitcoin/bitcoin/pull/28318#issuecomment-1856138054)
Big concept ACK on the interface. This is a nice simplification.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426942102)
Er, the ugly thing is testmempoolaccept... since `AcceptMultipleTransactions` doesn't actually enforce the child-with-parents thing, we can have this right now:
```python
@cleanup
def test_v3_in_testmempoolaccept(self):
node = self.nodes[0]
self.log.info("Test that v3 inheritance is accurately assessed in testmempoolaccept")
tx_v2 = self.wallet.create_self_transfer(version=2)
tx_v2_from_v2 = self.wallet.create_self_transfer(utxo_to_spend=tx_v2["
...
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1426942102)
Er, the ugly thing is testmempoolaccept... since `AcceptMultipleTransactions` doesn't actually enforce the child-with-parents thing, we can have this right now:
```python
@cleanup
def test_v3_in_testmempoolaccept(self):
node = self.nodes[0]
self.log.info("Test that v3 inheritance is accurately assessed in testmempoolaccept")
tx_v2 = self.wallet.create_self_transfer(version=2)
tx_v2_from_v2 = self.wallet.create_self_transfer(utxo_to_spend=tx_v2["
...
💬 maflcko commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1856146425)
I was looking in moving boost signals2 into one translation unit (because it is so heavy and keeps the compiler busy), to only expose the stuff that is needed, but if it is possible to re-implement signals2 in 150 LOC, that seems preferable?
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1856146425)
I was looking in moving boost signals2 into one translation unit (because it is so heavy and keeps the compiler busy), to only expose the stuff that is needed, but if it is possible to re-implement signals2 in 150 LOC, that seems preferable?
💬 fanquake commented on pull request "ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1856150718)
> We do not use any of other tools preinstalled via Homebrew.
Don't we use at least git, curl and Python? If any those are probably less-prone to breakage.
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1856150718)
> We do not use any of other tools preinstalled via Homebrew.
Don't we use at least git, curl and Python? If any those are probably less-prone to breakage.
💬 LaurentMT commented on issue "Improving fee estimation accuracy":
(https://github.com/bitcoin/bitcoin/issues/27995#issuecomment-1856162406)
A minimalist implementation of the second idea ("look at the rate and feerate of transactions entering the mempool currently"): https://code.samourai.io/oxt/one_dollar_fee_estimator
From our observations, the estimator behaves rather well under current network conditions but as noted in the original post, no fee estimator should be used automatically without safeguards put in place.
(https://github.com/bitcoin/bitcoin/issues/27995#issuecomment-1856162406)
A minimalist implementation of the second idea ("look at the rate and feerate of transactions entering the mempool currently"): https://code.samourai.io/oxt/one_dollar_fee_estimator
From our observations, the estimator behaves rather well under current network conditions but as noted in the original post, no fee estimator should be used automatically without safeguards put in place.
💬 theuni commented on pull request "Bump minimum required Boost version due to migration to C++20":
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1856193772)
@maflcko IIRC I wanted to fix #26442 before going forward with the replacement, as that makes the implementation more straightforward and greatly simplifies testing. Any interest in taking a look there?
(https://github.com/bitcoin/bitcoin/pull/29066#issuecomment-1856193772)
@maflcko IIRC I wanted to fix #26442 before going forward with the replacement, as that makes the implementation more straightforward and greatly simplifies testing. Any interest in taking a look there?