⚠️ maflcko opened an issue: "test: intermittent issue in p2p_1p1c_network.py"
(https://github.com/bitcoin/bitcoin/issues/31946)
> https://github.com/bitcoin/bitcoin/actions/runs/13498602529/job/37711361328?pr=31603#step:6:3286
_Originally posted by @brunoerg in [#31721](https://github.com/bitcoin/bitcoin/issues/31721#issuecomment-2678560262)_
(https://github.com/bitcoin/bitcoin/issues/31946)
> https://github.com/bitcoin/bitcoin/actions/runs/13498602529/job/37711361328?pr=31603#step:6:3286
_Originally posted by @brunoerg in [#31721](https://github.com/bitcoin/bitcoin/issues/31721#issuecomment-2678560262)_
💬 maflcko commented on pull request "test: create assert_not_equal util":
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1967842752)
> Distinguishing itself from `assert` which is skipped when running Python with `-O`.
It is not possible to run the functional tests with `-O` (they'd fail on current master), and it is unclear what reason there could be to do so in the first place. So i think using the flag as an argument for a code change seems weak.
If there is a reason for `-O`, it should be mentioned, all tests should be fixed, and it should be enforced with something like https://docs.astral.sh/ruff/rules/assert/.
...
(https://github.com/bitcoin/bitcoin/pull/29500#discussion_r1967842752)
> Distinguishing itself from `assert` which is skipped when running Python with `-O`.
It is not possible to run the functional tests with `-O` (they'd fail on current master), and it is unclear what reason there could be to do so in the first place. So i think using the flag as an argument for a code change seems weak.
If there is a reason for `-O`, it should be mentioned, all tests should be fixed, and it should be enforced with something like https://docs.astral.sh/ruff/rules/assert/.
...
⚠️ maflcko opened an issue: "clang ubsan build fails link step after cmake migration"
(https://github.com/bitcoin/bitcoin/issues/31947)
The error only happens for cmake builds, starting with the cmake migration.
Steps to reproduce on Debian/Ubuntu:
* Install packages and clone repo: `( export DEBIAN_FRONTEND=noninteractive && apt update && apt install git ccache make build-essential libtool cmake autotools-dev automake pkg-config bsdmainutils python3 libevent-dev libboost-dev libsqlite3-dev clang llvm -y && git clone https://github.com/bitcoin/bitcoin.git b-c ) && cd b-c `
* `git checkout 338bc2cd261ba3daf7fb494f8cb4a534
...
(https://github.com/bitcoin/bitcoin/issues/31947)
The error only happens for cmake builds, starting with the cmake migration.
Steps to reproduce on Debian/Ubuntu:
* Install packages and clone repo: `( export DEBIAN_FRONTEND=noninteractive && apt update && apt install git ccache make build-essential libtool cmake autotools-dev automake pkg-config bsdmainutils python3 libevent-dev libboost-dev libsqlite3-dev clang llvm -y && git clone https://github.com/bitcoin/bitcoin.git b-c ) && cd b-c `
* `git checkout 338bc2cd261ba3daf7fb494f8cb4a534
...
💬 maflcko commented on issue "clang ubsan build fails link step after cmake migration":
(https://github.com/bitcoin/bitcoin/issues/31947#issuecomment-2678937391)
Sorry, wrong alarm
(https://github.com/bitcoin/bitcoin/issues/31947#issuecomment-2678937391)
Sorry, wrong alarm
✅ maflcko closed an issue: "clang ubsan build fails link step after cmake migration"
(https://github.com/bitcoin/bitcoin/issues/31947)
(https://github.com/bitcoin/bitcoin/issues/31947)
💬 glozow commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1967955532)
Perhaps include what the min and max are.
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1967955532)
Perhaps include what the min and max are.
🤔 glozow reviewed a pull request: "rpc: Support v3 raw transactions creation"
(https://github.com/bitcoin/bitcoin/pull/31936#pullrequestreview-2637660876)
Concept ACK. Thanks for your PR, but it needs to pass CI and new features need tests to be considered (please reading CONTRIBUTING.md). Perhaps a functional test in rpc_rawtransaction.py.
(https://github.com/bitcoin/bitcoin/pull/31936#pullrequestreview-2637660876)
Concept ACK. Thanks for your PR, but it needs to pass CI and new features need tests to be considered (please reading CONTRIBUTING.md). Perhaps a functional test in rpc_rawtransaction.py.
💬 glozow commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1967964337)
Why `int64_t`? `version` has type `uint32_t`.
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1967964337)
Why `int64_t`? `version` has type `uint32_t`.
💬 glozow commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1967957006)
Why aren't we using `TX_MAX_STANDARD_VERSION` everywhere?
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r1967957006)
Why aren't we using `TX_MAX_STANDARD_VERSION` everywhere?
💬 glozow commented on pull request "fuzz: add basic TxOrphanage::EraseForBlock cov":
(https://github.com/bitcoin/bitcoin/pull/31918#discussion_r1967976836)
Maybe would have been cool to make the fuzzer generate a tx, that way can have non-identical but conflicting txns.
(https://github.com/bitcoin/bitcoin/pull/31918#discussion_r1967976836)
Maybe would have been cool to make the fuzzer generate a tx, that way can have non-identical but conflicting txns.
🤔 glozow reviewed a pull request: "fuzz: add basic TxOrphanage::EraseForBlock cov"
(https://github.com/bitcoin/bitcoin/pull/31918#pullrequestreview-2637698542)
post merge code review ACK
(https://github.com/bitcoin/bitcoin/pull/31918#pullrequestreview-2637698542)
post merge code review ACK
💬 tnndbtc commented on issue "build: macOS fuzz instructions broken using latest macOS linker":
(https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2678994353)
@maflcko Instead of hard-code to a specific version, I believe the right instruction for Mac is to run "ld -v" from console. Then, depend on the output of "Apple TAPI version x.x.x", install the right llvm version accordingly.
(https://github.com/bitcoin/bitcoin/issues/31049#issuecomment-2678994353)
@maflcko Instead of hard-code to a specific version, I believe the right instruction for Mac is to run "ld -v" from console. Then, depend on the output of "Apple TAPI version x.x.x", install the right llvm version accordingly.
🤔 pablomartin4btc reviewed a pull request: "refactor: Remove redundant and confusing calls to IsArgSet"
(https://github.com/bitcoin/bitcoin/pull/31896#pullrequestreview-2637657603)
cr & tACK fa3fb3c23fae287b161112b9b04cf0937a1051c6
What about the `IsArgSet` ones followed by `GetPathArg`?
(https://github.com/bitcoin/bitcoin/pull/31896#pullrequestreview-2637657603)
cr & tACK fa3fb3c23fae287b161112b9b04cf0937a1051c6
What about the `IsArgSet` ones followed by `GetPathArg`?
💬 pablomartin4btc commented on pull request "refactor: Remove redundant and confusing calls to IsArgSet":
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r1967953596)
Also redundant/ repeated calls to `args.GetArg` were removed...
(https://github.com/bitcoin/bitcoin/pull/31896#discussion_r1967953596)
Also redundant/ repeated calls to `args.GetArg` were removed...
💬 hodlinator commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967897709)
Curious why you include `RUST_BACKTRACE=1` in the recommended command line?
Also could maybe use angle-brackets or something to denote input?
```suggestion
cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build_dir <boost_unittest_filter>
```
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967897709)
Curious why you include `RUST_BACKTRACE=1` in the recommended command line?
Also could maybe use angle-brackets or something to denote input?
```suggestion
cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build_dir <boost_unittest_filter>
```
👍 hodlinator approved a pull request: "contrib: Add deterministic-unittest-coverage"
(https://github.com/bitcoin/bitcoin/pull/31901#pullrequestreview-2637311191)
ACK fafa592a8ad95fcb64db3d342cdc7192ff7ef2cd
Was able to use this to diagnose and disable non-determinism issues for:
`cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_tests
`
<details><summary>Hacky WIP pro-determinism patch resulting in pass</summary>
```diff
diff --git a/src/random.cpp b/src/random.cpp
index 5b605e988d..087894ffcd 100644
--- a/src/random.cpp
+++ b/src/random.cpp
@@ -441,6 +441,8 @@ public:
~RNG
...
(https://github.com/bitcoin/bitcoin/pull/31901#pullrequestreview-2637311191)
ACK fafa592a8ad95fcb64db3d342cdc7192ff7ef2cd
Was able to use this to diagnose and disable non-determinism issues for:
`cargo run --manifest-path ./contrib/devtools/deterministic-unittest-coverage/Cargo.toml -- $PWD/build util_tests
`
<details><summary>Hacky WIP pro-determinism patch resulting in pass</summary>
```diff
diff --git a/src/random.cpp b/src/random.cpp
index 5b605e988d..087894ffcd 100644
--- a/src/random.cpp
+++ b/src/random.cpp
@@ -441,6 +441,8 @@ public:
~RNG
...
💬 hodlinator commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967952190)
nit: Could provide a more satisfying message.
```suggestion
println!("Coverage test passed - determinism held for given conditions in 2 consecutive runs!");
```
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967952190)
nit: Could provide a more satisfying message.
```suggestion
println!("Coverage test passed - determinism held for given conditions in 2 consecutive runs!");
```
💬 hodlinator commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967761569)
Responding to https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967697007:
> I don't think this has something to do with the language it is written in. I am sure it is possible to write a check for the exit code in any language and print a different error message than an assertion failure.
Bash can't spew stacktraces AFAIK. And the "noise" is even larger than I [posted initially](https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1966329661), if one runs with the recommende
...
(https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967761569)
Responding to https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1967697007:
> I don't think this has something to do with the language it is written in. I am sure it is possible to write a check for the exit code in any language and print a different error message than an assertion failure.
Bash can't spew stacktraces AFAIK. And the "noise" is even larger than I [posted initially](https://github.com/bitcoin/bitcoin/pull/31901#discussion_r1966329661), if one runs with the recommende
...
👍 pablomartin4btc approved a pull request: "test: fix TestShell initialization and reset()"
(https://github.com/bitcoin/bitcoin/pull/31415#pullrequestreview-2637840124)
re-ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d
(reviewed changes since last ACK: update to `build/test` paths & reverted global var move)
(https://github.com/bitcoin/bitcoin/pull/31415#pullrequestreview-2637840124)
re-ACK 303f8cca0568d2ca77189c4eccdfc7a6913c958d
(reviewed changes since last ACK: update to `build/test` paths & reverted global var move)
💬 glozow commented on issue "Ephemeral Dust 0-Fee Requirement Complexifies Downstream Protocols":
(https://github.com/bitcoin/bitcoin/issues/31938#issuecomment-2679414095)
So to summarize. Given you have `amt` amount of trimmed htlcs to burn, this is the "annoying" option you'd need to do now:
- `amt > 0 and <= dust`: put it in the anchor
- `amt > dust`: anchor has `dust`, fees have `amt - dust`
And the option you want: just put 0 in the anchor, everything else into fees. So this is kind of like removing the dust limit (apart from the rule that you can only have 1).
> they'd be incentivized to go use a private mempool service (eg MEVPool) which is bad for everyo
...
(https://github.com/bitcoin/bitcoin/issues/31938#issuecomment-2679414095)
So to summarize. Given you have `amt` amount of trimmed htlcs to burn, this is the "annoying" option you'd need to do now:
- `amt > 0 and <= dust`: put it in the anchor
- `amt > dust`: anchor has `dust`, fees have `amt - dust`
And the option you want: just put 0 in the anchor, everything else into fees. So this is kind of like removing the dust limit (apart from the rule that you can only have 1).
> they'd be incentivized to go use a private mempool service (eg MEVPool) which is bad for everyo
...