💬 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?
👍 TheCharlatan approved a pull request: "wallet: Implement independent BDB parser"
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-1782050983)
Re-ACK 3c91202c5a03c57c9f83186de9d3caf337b78d45
(https://github.com/bitcoin/bitcoin/pull/26606#pullrequestreview-1782050983)
Re-ACK 3c91202c5a03c57c9f83186de9d3caf337b78d45
💬 TheCharlatan commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1426859564)
This comment is confusing, aren't we going from `BERKELEY` to `BERKELEY_RO`?
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1426859564)
This comment is confusing, aren't we going from `BERKELEY` to `BERKELEY_RO`?
💬 hebasto commented on pull request "ci: Set `HOMEBREW_NO_INSTALLED_DEPENDENTS_CHECK` to avoid unrelated failures":
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1856204103)
> Don't we use at least git, curl and Python?
Right. Here is the `brew list -1` command output for the image version `20231025.2`:
```
ant
aom
apr
apr-util
argon2
aria2
aspell
autoconf
aws-sam-cli
azure-cli
bazelisk
berkeley-db
bicep
brotli
c-ares
ca-certificates
cairo
carthage
cffi
cmake
composer
curl
fontconfig
freetds
freetype
gcc
gcc@11
gcc@12
gd
gdbm
geckodriver
gettext
gh
giflib
git
git-lfs
glib
gmp
gnu-tar
gnupg
gnutls
gradle
graphite2
harfb
...
(https://github.com/bitcoin/bitcoin/pull/29080#issuecomment-1856204103)
> Don't we use at least git, curl and Python?
Right. Here is the `brew list -1` command output for the image version `20231025.2`:
```
ant
aom
apr
apr-util
argon2
aria2
aspell
autoconf
aws-sam-cli
azure-cli
bazelisk
berkeley-db
bicep
brotli
c-ares
ca-certificates
cairo
carthage
cffi
cmake
composer
curl
fontconfig
freetds
freetype
gcc
gcc@11
gcc@12
gd
gdbm
geckodriver
gettext
gh
giflib
git
git-lfs
glib
gmp
gnu-tar
gnupg
gnutls
gradle
graphite2
harfb
...
📝 maflcko opened a pull request: "refactor: Remove gmtime*"
(https://github.com/bitcoin/bitcoin/pull/29081)
Now that the `ChronoSanityCheck` has passed for everyone with C++17 and is guaranteed by C++20 to always pass, remove it.
Also, remove `gmtime_r` and `gmtime_s` and replace them with `year_month_day`+`hh_mm_ss` from C++20.
(https://github.com/bitcoin/bitcoin/pull/29081)
Now that the `ChronoSanityCheck` has passed for everyone with C++17 and is guaranteed by C++20 to always pass, remove it.
Also, remove `gmtime_r` and `gmtime_s` and replace them with `year_month_day`+`hh_mm_ss` from C++20.
💬 maflcko commented on pull request "refactor: Replace GetTimeMicros by SystemClock":
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1427019264)
Fixed in https://github.com/bitcoin/bitcoin/pull/29081
(https://github.com/bitcoin/bitcoin/pull/27233#discussion_r1427019264)
Fixed in https://github.com/bitcoin/bitcoin/pull/29081
💬 nafisalawalidris commented on issue "test: Write assumeutxo tests":
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1856297893)
Hello @maflcko, I'm interested in working on this issue and adding tests for the AssumeUtxo feature. I will follow the guidance provided in the issue description and the template for writing tests. I will keep you posted on my progress and submit a pull request once the tests are ready.
(https://github.com/bitcoin/bitcoin/issues/28648#issuecomment-1856297893)
Hello @maflcko, I'm interested in working on this issue and adding tests for the AssumeUtxo feature. I will follow the guidance provided in the issue description and the template for writing tests. I will keep you posted on my progress and submit a pull request once the tests are ready.