π¬ MarcoFalke commented on pull request "test: move remaining rand code from util/setup_common to util/random":
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1250420188)
in 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a:
Some notes:
* I wonder if this should still be called "insecure", as the underlying randomness is no longer insecure, just deterministic, which is obvious in a test env. Maybe call it MockedRandBytes?
* `static` isn't needed?
* I wonder if it makes sense to wrap all member functions to global functions. Why not just call the member function of the global? (Before maybe renaming it to `g_mocked_random` or `g_test_random` or so? This would allow
...
(https://github.com/bitcoin/bitcoin/pull/27425#discussion_r1250420188)
in 5b3f6a49e6bda9f5a0ec261f3f8900e4978e3a1a:
Some notes:
* I wonder if this should still be called "insecure", as the underlying randomness is no longer insecure, just deterministic, which is obvious in a test env. Maybe call it MockedRandBytes?
* `static` isn't needed?
* I wonder if it makes sense to wrap all member functions to global functions. Why not just call the member function of the global? (Before maybe renaming it to `g_mocked_random` or `g_test_random` or so? This would allow
...
π¬ S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246207242)
in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893
I think all the tests would be easier to understand if we start with an empty testing wallet and send unconfirmed txs to it from `def_wallet`.
Now we start with the testing wallet with already one confirmed input, but why if we test spending mostly unconfirmed txs? Current setup requires reader to do more coin tracking in mind.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246207242)
in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893
I think all the tests would be easier to understand if we start with an empty testing wallet and send unconfirmed txs to it from `def_wallet`.
Now we start with the testing wallet with already one confirmed input, but why if we test spending mostly unconfirmed txs? Current setup requires reader to do more coin tracking in mind.
π€ S3RK reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1504669255)
Started reviewing tests, but need more time.
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1504669255)
Started reviewing tests, but need more time.
π¬ S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246215741)
do we want to check naΓ―ve ancestry fee rate or the fee_rate at which it will be accepted at a block (i.e. ancestry score)?
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246215741)
do we want to check naΓ―ve ancestry fee rate or the fee_rate at which it will be accepted at a block (i.e. ancestry score)?
π¬ S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246199879)
in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893
nit: better use a multiple of `target_fee_rate`
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1246199879)
in "Bump unconfirmed parent txs to target feerate" 8e3d924f02ee28043e8844a301389915823e5893
nit: better use a multiple of `target_fee_rate`
π¬ S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250425745)
that seems like a duplicate check of `assert_undershoots_target`
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250425745)
that seems like a duplicate check of `assert_undershoots_target`
π¬ S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250394244)
Oops, sorry about misleading you π
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250394244)
Oops, sorry about misleading you π
π¬ S3RK commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250419037)
My idea was to add `bump_fee_group_discount` within `GetTotalBumpFee()` function. Probably I'm missing something but I don't know what. Why wouldn't it just work?
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1250419037)
My idea was to add `bump_fee_group_discount` within `GetTotalBumpFee()` function. Probably I'm missing something but I don't know what. Why wouldn't it just work?
π fanquake opened a pull request: "docs: fixup honggfuzz fuzz patch"
(https://github.com/bitcoin/bitcoin/pull/28021)
Closes #28019.
(https://github.com/bitcoin/bitcoin/pull/28021)
Closes #28019.
π¬ MarcoFalke commented on pull request "docs: fixup honggfuzz fuzz patch":
(https://github.com/bitcoin/bitcoin/pull/28021#discussion_r1250601396)
Any reason to make the comment wrong?
(https://github.com/bitcoin/bitcoin/pull/28021#discussion_r1250601396)
Any reason to make the comment wrong?
π¬ fanquake commented on pull request "docs: fixup honggfuzz fuzz patch":
(https://github.com/bitcoin/bitcoin/pull/28021#discussion_r1250606783)
Just a miss-copy and paste
(https://github.com/bitcoin/bitcoin/pull/28021#discussion_r1250606783)
Just a miss-copy and paste
π MarcoFalke approved a pull request: "docs: fixup honggfuzz fuzz patch"
(https://github.com/bitcoin/bitcoin/pull/28021#pullrequestreview-1510847799)
lgtm
(https://github.com/bitcoin/bitcoin/pull/28021#pullrequestreview-1510847799)
lgtm
π¬ stickies-v commented on issue "rpc: doc: RPCHelpMan missing request parameters for getblocktemplate":
(https://github.com/bitcoin/bitcoin/issues/27998#issuecomment-1617805106)
Absolutely, just open the PR whenever you feel ready, or ask away here if you have any problems you can't figure out.
(https://github.com/bitcoin/bitcoin/issues/27998#issuecomment-1617805106)
Absolutely, just open the PR whenever you feel ready, or ask away here if you have any problems you can't figure out.
π brunoerg approved a pull request: "docs: fixup honggfuzz fuzz patch"
(https://github.com/bitcoin/bitcoin/pull/28021#pullrequestreview-1510913912)
ACK c1247c3746d4b9ea88a0f9cfb7e71904267b3cd3
(https://github.com/bitcoin/bitcoin/pull/28021#pullrequestreview-1510913912)
ACK c1247c3746d4b9ea88a0f9cfb7e71904267b3cd3
β
fanquake closed an issue: "docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated"
(https://github.com/bitcoin/bitcoin/issues/28019)
(https://github.com/bitcoin/bitcoin/issues/28019)
π fanquake merged a pull request: "docs: fixup honggfuzz fuzz patch"
(https://github.com/bitcoin/bitcoin/pull/28021)
(https://github.com/bitcoin/bitcoin/pull/28021)
π¬ fanquake commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1618004102)
> Does it mean we drop support for distro-shipped valgrind?
I think they should remain supported similar to what we do now, where we roughly support recent Valgrind versions on recent distro releases, when combined with recent compilers etc, and we still see occasional issues i.e #27741.
> I checked the debian/rules file and did not see anything that we might be missing there in terms of build configuration.
I posted a little more about this here: https://github.com/bitcoin-core/qa-ass
...
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1618004102)
> Does it mean we drop support for distro-shipped valgrind?
I think they should remain supported similar to what we do now, where we roughly support recent Valgrind versions on recent distro releases, when combined with recent compilers etc, and we still see occasional issues i.e #27741.
> I checked the debian/rules file and did not see anything that we might be missing there in terms of build configuration.
I posted a little more about this here: https://github.com/bitcoin-core/qa-ass
...
π€ t-bast reviewed a pull request: "Bump unconfirmed ancestor transactions to target feerate"
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1511014531)
I tried to run my own set of external e2e tests against https://github.com/bitcoin/bitcoin/pull/26152/commits/43b86e49351b3160c2e39807997a9373cd88e173 and they all hit the following assertion error:
```
bitcoind: wallet/coinselection.cpp:495: void wallet::SelectionResult::SetBumpFeeDiscount(CAmount): Assertion `discount >= 0' failed
```
Reading through recent comments, it seems somewhat expected that this version isn't yet fully working, let me know when you want me to run my tests again
...
(https://github.com/bitcoin/bitcoin/pull/26152#pullrequestreview-1511014531)
I tried to run my own set of external e2e tests against https://github.com/bitcoin/bitcoin/pull/26152/commits/43b86e49351b3160c2e39807997a9373cd88e173 and they all hit the following assertion error:
```
bitcoind: wallet/coinselection.cpp:495: void wallet::SelectionResult::SetBumpFeeDiscount(CAmount): Assertion `discount >= 0' failed
```
Reading through recent comments, it seems somewhat expected that this version isn't yet fully working, let me know when you want me to run my tests again
...
π¬ hebasto commented on pull request "Drop macOS ForceActivation workaround":
(https://github.com/bitcoin-core/gui/pull/744#issuecomment-1618090522)
Concept ACK.
> We now require 5.11.x+.
When building on macOS, Homebrew provides Qt [5.15.10](https://formulae.brew.sh/formula/qt@5). Our depends does 5.15.5.
cc @Sjors
(https://github.com/bitcoin-core/gui/pull/744#issuecomment-1618090522)
Concept ACK.
> We now require 5.11.x+.
When building on macOS, Homebrew provides Qt [5.15.10](https://formulae.brew.sh/formula/qt@5). Our depends does 5.15.5.
cc @Sjors
π criadoperez opened a pull request: "typos on src files"
(https://github.com/bitcoin/bitcoin/pull/28022)
Fixes multiple typos
(https://github.com/bitcoin/bitcoin/pull/28022)
Fixes multiple typos